General Edit User Improvements #1779
Conversation
📝 WalkthroughWalkthroughCentralizes OAuth linking/unlinking into OAuthService (new linkUser/unlinkUser) and updates controller/UI to use it. Converts User admin form to tabbed layout with external_id, timezone/language, avatar, OAuth unlink, API/SSH keys, and password reset. Adjusts avatar handling and gates ActivityLog IP display by permission. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Provider as OAuth Provider
participant App as Application
participant C as OAuthController
participant S as OAuthService
participant DB as Database
User->>App: Start OAuth flow (login or link)
App->>Provider: Redirect to provider auth
Provider-->>App: Callback with code
App->>C: handleCallback(request)
C->>Provider: fetch OAuth user (Socialite)
C->>S: linkUser(authenticated User, Schema, OAuthUser)
S->>DB: persist user's oauth map update
DB-->>S: persisted
S-->>C: return refreshed User
C-->>App: continue flow (redirect/response)
sequenceDiagram
autonumber
actor Admin
participant UI as Filament UI (EditUser/EditProfile)
participant S as OAuthService
participant DB as Database
participant FS as File Storage
rect rgba(230,245,255,0.6)
Admin->>UI: Click "Unlink" for provider
UI->>S: unlinkUser(current User, Schema)
S->>DB: remove schema entry from user's oauth map
DB-->>S: persisted
S-->>UI: return refreshed User
UI-->>Admin: show notification
end
rect rgba(245,245,245,0.6)
Admin->>UI: Remove avatar
alt Temporary upload
UI->>FS: Delete temp file directly
else Stored avatar path
UI->>FS: Delete disk file
end
UI->>DB: clear avatar path on user
DB-->>UI: saved
end
Possibly related PRs
Pre-merge checks❌ Failed checks (3 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
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 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: 7
🧹 Nitpick comments (3)
app/Extensions/OAuth/OAuthService.php (1)
56-56: Consider if refresh() is necessary.Both
linkUser(line 56) andunlinkUser(line 69) call$user->refresh()after updating. Sinceupdate()modifies the database but doesn't automatically reload all attributes, therefresh()ensures the returned user instance reflects the updated state. However, if the only change is to theoauthattribute and it was explicitly set viaupdate(['oauth' => $oauth]), the refresh may not be strictly necessary unless other model observers or events modify additional attributes.If you're certain no other attributes are modified during the update (e.g., via model events or observers), you could simplify by removing the
refresh()call:public function linkUser(User $user, OAuthSchemaInterface $schema, OAuthUser $oauthUser): User { $oauth = $user->oauth ?? []; $oauth[$schema->getId()] = $oauthUser->getId(); $user->update(['oauth' => $oauth]); - return $user->refresh(); + return $user; }However, keeping
refresh()is safer and ensures consistency if there are any hidden side effects from observers or database triggers.Also applies to: 69-69
app/Filament/Admin/Resources/Users/UserResource.php (2)
169-172: Unique validators should ignore the current recordAvoid false “already taken” on edit by making the rule ignorable for the current model.
Based on learnings
- ->unique() + ->unique(ignorable: fn (?User $record) => $record) ... - ->unique() + ->unique(ignorable: fn (?User $record) => $record)Please verify if your Filament v4 version already defaults to ignoring the current record; if not, apply this.
Also applies to: 179-183
265-267: Optional: cache timezone optionsBuilding the full timezone list on every render is unnecessary. Cache the options statically/once per request.
-->options(fn () => collect(DateTimeZone::listIdentifiers())->mapWithKeys(fn ($tz) => [$tz => $tz])) +->options(function () { + static $opts; + return $opts ??= collect(DateTimeZone::listIdentifiers())->mapWithKeys(fn ($tz) => [$tz => $tz]); +})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/Extensions/OAuth/OAuthService.php(2 hunks)app/Filament/Admin/Resources/Users/Pages/EditUser.php(1 hunks)app/Filament/Admin/Resources/Users/UserResource.php(2 hunks)app/Filament/Pages/Auth/EditProfile.php(3 hunks)app/Filament/Server/Resources/Activities/ActivityResource.php(1 hunks)app/Http/Controllers/Auth/OAuthController.php(2 hunks)app/Models/ActivityLog.php(3 hunks)lang/en/admin/user.php(1 hunks)lang/en/profile.php(1 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
app/Filament/Admin/Resources/Users/UserResource.php
311-311: Avoid unused local variables such as '$_'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (12)
app/Filament/Server/Resources/Activities/ActivityResource.php (1)
82-82: LGTM! Centralized IP permission check.The refactor to always call
$activityLog->getIp()is correct. The permission check is now encapsulated within theActivityLog::getIp()method (lines 166-169 in app/Models/ActivityLog.php), which returnsnullwhen the user lacks the 'seeIps activityLog' permission. This improves code organization by centralizing the authorization logic.app/Models/ActivityLog.php (2)
166-169: LGTM! Permission-based IP accessor.The new
getIp()method correctly encapsulates the permission check for viewing activity log IPs. Returningnullwhen the user lacks permission is the appropriate behavior, as it prevents IP exposure while allowing the calling code to handle the absence of data gracefully.
183-184: LGTM! Safe IP formatting with fallback.The IP formatting logic correctly handles the
nullcase fromgetIp()by converting it to an empty string. The ternary operator ensures that when the IP is not visible (null), no separator is appended, resulting in a clean display.app/Http/Controllers/Auth/OAuthController.php (2)
59-59: LGTM! Centralized OAuth linking.The refactor to delegate OAuth user linking to
OAuthService::linkUser()improves code organization by centralizing OAuth management logic. This eliminates duplication and ensures consistent behavior across the application.
86-86: LGTM! Consistent OAuth service usage.The delegation to
$this->oauthService->linkUser()for handling missing users is consistent with the approach used in the callback method (line 59). This ensures uniform OAuth linking behavior throughout the controller.lang/en/admin/user.php (1)
12-12: LGTM! New translation keys for user management features.The added translation keys support the new features mentioned in the PR objectives:
external_idfor displaying OAuth external identifierspassword_reset,password_reset_sent, andpassword_reset_failedfor the password reset functionalityThe keys are clear, descriptive, and follow the existing naming conventions.
Also applies to: 19-21
app/Filament/Pages/Auth/EditProfile.php (2)
154-168: LGTM! Improved avatar file handling.The updated avatar file upload handling correctly:
- Uses
formatStateUsingto populate the field with the existing avatar path when present (lines 154-159)- Implements
deleteUploadedFileUsingto handle both temporary uploads (TemporaryUploadedFile) and stored files (lines 160-168)This provides proper lifecycle management for avatar files, ensuring temporary uploads are cleaned up and existing avatars can be removed.
188-190: LGTM! Centralized OAuth unlinking.The refactor to use
$this->oauthService->unlinkUser()(line 190) aligns with the centralized OAuth management approach introduced in this PR. This eliminates manual array manipulation and ensures consistent behavior with other OAuth operations throughout the application.lang/en/profile.php (1)
11-11: 'keys' translation is required. It’s used as the parent tab label grouping both API and SSH keys in the profile UI, so it isn’t redundant.app/Filament/Admin/Resources/Users/UserResource.php (3)
391-396: Double subject in Activity chain — confirm API semanticsCalling ->subject($user) then ->subject($apiKey/$sshKey) likely overwrites the first. If your Activity facade supports only a single subject, consider using actor/subject consistently and move the other value into ->property().
Do you want me to adjust these calls to your facade’s canonical pattern?
Also applies to: 429-434
460-463: Confirm Repeater relationship usagerelationship(null, ...) relies on implicit naming. Ensure this binds to the intended relationship on User (e.g., activity or activityLogs) and works with the v4 Schemas API. If not, pass the explicit relationship name.
Based on learnings
285-304: Avatar assumes.pngstorageVisibility/formatStateUsing/delete logic is hardcoded to {id}.png on the public disk. Confirm the avatar pipeline always normalizes to PNG; otherwise deletions may miss JPEG/WebP.
If formats differ, I can update this to resolve the actual stored path and delete variants.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
app/Filament/Admin/Resources/Users/UserResource.php (4)
339-339: Invalid query builder methodwhereNot.Laravel's Eloquent query builder doesn't provide a
whereNot()method. This will cause a runtime error.Apply this diff to use a valid query method:
-->relationship('roles', 'name', fn (Builder $query) => $query->whereNot('id', Role::getRootAdmin()->id)) +->relationship('roles', 'name', fn (Builder $query) => $query->where('id', '!=', Role::getRootAdmin()->id))Alternatively, use
whereKeyNot:-->relationship('roles', 'name', fn (Builder $query) => $query->whereNot('id', Role::getRootAdmin()->id)) +->relationship('roles', 'name', fn (Builder $query) => $query->whereKeyNot(Role::getRootAdmin()->id))
414-424: Fix null dereference in SSH key delete action.Same issue as with API keys:
UserSSHKey::find()can returnnull, and calling->exists()onnullwill throw an error.Apply this diff to fix the null check:
$sshKey = UserSSHKey::find($key['id'] ?? null); -if ($sshKey->exists()) { +if ($sshKey) { $sshKey->delete(); Activity::event('user:ssh-key.delete') ->actor(auth()->user()) ->subject($user) ->subject($sshKey) ->property('fingerprint', $sshKey->fingerprint) ->log(); }
376-387: Fix null dereference in API key delete action.
ApiKey::find()can returnnull, and calling->exists()onnullwill throw an error. Additionally,existsis a property on Eloquent models, not a method.Apply this diff to fix the null check:
$apiKey = ApiKey::find($key['id'] ?? null); -if ($apiKey->exists()) { +if ($apiKey) { $apiKey->delete(); Activity::event('user:api-key.delete') ->actor(auth()->user()) ->subject($user) ->subject($apiKey) ->property('identifier', $apiKey->identifier) ->log(); }
307-331: Fix variable shadowing, early return, and missing refresh.This code has several issues that were previously flagged but remain unaddressed:
- Variable shadowing: Line 308 overwrites the loop variable
$schema(which holds the schema key) with the schema object, making the key inaccessible.- Early return aborts action building: Line 310's
returnexits the entire closure, preventing subsequent OAuth providers from being processed. Should becontinue.- Missing refresh before form refill: Line 322 refills the form without refreshing the user model, potentially showing stale data.
Apply this diff to fix all issues:
-foreach ($user->oauth as $schema => $_) { - $schema = $oauthService->get($schema); - if (!$schema) { - return; +foreach (array_keys($user->oauth ?? []) as $schemaKey) { + $provider = $oauthService->get($schemaKey); + if (!$provider) { + continue; } - $id = $schema->getId(); - $name = $schema->getName(); + $id = $provider->getId(); + $name = $provider->getName(); $actions[] = Action::make("oauth_$id") ->label(trans('profile.unlink', ['name' => $name])) ->icon('tabler-unlink') ->requiresConfirmation() - ->color(Color::hex($schema->getHexColor())) - ->action(function ($livewire) use ($oauthService, $user, $name, $schema) { - $oauthService->unlinkUser($user, $schema); + ->color(Color::hex($provider->getHexColor())) + ->action(function ($livewire) use ($oauthService, $user, $name, $provider) { + $oauthService->unlinkUser($user, $provider); + $user->refresh(); $livewire->form->fill($user->attributesToArray()); Notification::make() ->title(trans('profile.unlinked', ['name' => $name])) ->success() ->send(); }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Extensions/OAuth/OAuthService.php(2 hunks)app/Filament/Admin/Resources/Users/UserResource.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/Filament/Admin/Resources/Users/UserResource.php (8)
app/Extensions/OAuth/OAuthService.php (3)
OAuthService(10-71)get(21-24)unlinkUser(59-70)app/Facades/Activity.php (1)
Activity(8-14)app/Models/ActivityLog.php (4)
ActivityLog(55-265)apiKey(106-109)actor(93-96)htmlable(171-197)app/Models/ApiKey.php (1)
ApiKey(56-234)app/Models/User.php (1)
User(94-492)app/Models/UserSSHKey.php (1)
UserSSHKey(43-70)app/Notifications/MailTested.php (1)
MailTested(9-28)app/Services/Helpers/LanguageService.php (2)
LanguageService(8-30)getAvailableLanguages(22-29)
app/Extensions/OAuth/OAuthService.php (2)
app/Models/User.php (1)
User(94-492)app/Extensions/OAuth/OAuthSchemaInterface.php (1)
getId(10-10)
🪛 PHPMD (2.15.0)
app/Filament/Admin/Resources/Users/UserResource.php
307-307: Avoid unused local variables such as '$_'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (1)
app/Extensions/OAuth/OAuthService.php (1)
49-70: LGTM! Null handling correctly implemented.Both
linkUserandunlinkUsermethods now correctly handle the potential null value of$user->oauthusing the null coalescing operator (?? []). The implementations safely update the OAuth associations and return refreshed user instances.
There was a problem hiding this comment.
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/Filament/Pages/Auth/EditProfile.php (1)
188-204: Critical: undefined variable in OAuth unlink closure.The closure references
$updateServiceat line 193, but it's neither injected as a parameter nor captured in theuse()clause. This will cause a fatal error when the unlink action is triggered.Apply this diff to fix using the existing
OAuthService:- ->action(function () use ($id, $name, $schema, $unlink) { + ->action(function (OAuthService $oauthService) use ($id, $name, $schema, $unlink) { if ($unlink) { - $oauth = user()?->oauth; - unset($oauth[$id]); - - $updateService->handle(user(), ['oauth' => $oauth]); + $oauthService->unlinkUser(user(), $schema); $this->fillForm(); Notification::make() ->title(trans('profile.unlinked', ['name' => $name])) ->success() ->send(); } else { redirect(Socialite::with($id)->redirect()->getTargetUrl()); } });
♻️ Duplicate comments (5)
app/Filament/Admin/Resources/Users/UserResource.php (5)
339-339: Verify Laravel version forwhereNot()compatibility.The
whereNot()method was introduced in Laravel 10. Ensure your project is running Laravel 10 or higher, otherwise this will cause a fatal error.Run the following script to check the Laravel version:
#!/bin/bash # Check Laravel version echo "=== Checking Laravel version ===" cat composer.json | jq -r '.require["laravel/framework"]' echo -e "\n=== Checking composer.lock for installed version ===" cat composer.lock | jq -r '.packages[] | select(.name == "laravel/framework") | .version'If the version is below 10, apply this diff:
- ->relationship('roles', 'name', fn (Builder $query) => $query->whereNot('id', Role::getRootAdmin()->id)) + ->relationship('roles', 'name', fn (Builder $query) => $query->where('id', '!=', Role::getRootAdmin()->id))
307-331: Fix variable shadowing, early return, and add user refresh.Three issues remain from previous reviews:
- Line 307: Loop variable
$schemais shadowed when reassigned on line 308. Use a different variable name like$schemaKeyfor the loop.- Line 310:
return;exits the entire function, preventing subsequent OAuth providers from being processed. Usecontinue;instead.- Line 322: After unlinking, refresh the user model before refilling the form to ensure fresh data.
Apply this diff:
- foreach ($user->oauth as $schema => $_) { - $schema = $oauthService->get($schema); + foreach (array_keys($user->oauth ?? []) as $schemaKey) { + $provider = $oauthService->get($schemaKey); - if (!$schema) { - return; + if (!$provider) { + continue; } - $id = $schema->getId(); - $name = $schema->getName(); + $id = $provider->getId(); + $name = $provider->getName(); $actions[] = Action::make("oauth_$id") ->label(trans('profile.unlink', ['name' => $name])) ->icon('tabler-unlink') ->requiresConfirmation() - ->color(Color::hex($schema->getHexColor())) - ->action(function ($livewire) use ($oauthService, $user, $name, $schema) { - $oauthService->unlinkUser($user, $schema); + ->color(Color::hex($provider->getHexColor())) + ->action(function ($livewire) use ($oauthService, $user, $name, $provider) { + $oauthService->unlinkUser($user, $provider); + $user->refresh(); $livewire->form->fill($user->attributesToArray()); Notification::make() ->title(trans('profile.unlinked', ['name' => $name])) ->success() ->send(); }); }
376-387: Fix null pointer dereference in API key deletion.
ApiKey::find()returnsnullwhen the record is not found. Calling->exists()onnullwill throw a fatal error. Additionally, Eloquent models use the$model->existsproperty, not->exists()method.Apply this diff:
$apiKey = ApiKey::find($key['id'] ?? null); - if ($apiKey->exists()) { + if ($apiKey) { $apiKey->delete(); Activity::event('user:api-key.delete') ->actor(auth()->user()) ->subject($user) ->subject($apiKey) ->property('identifier', $apiKey->identifier) ->log(); }
414-424: Fix null pointer dereference in SSH key deletion.Same issue as the API key deletion:
UserSSHKey::find()can returnnull, and calling->exists()onnullwill throw a fatal error.Apply this diff:
$sshKey = UserSSHKey::find($key['id'] ?? null); - if ($sshKey->exists()) { + if ($sshKey) { $sshKey->delete(); Activity::event('user:ssh-key.delete') ->actor(auth()->user()) ->subject($user) ->subject($sshKey) ->property('fingerprint', $sshKey->fingerprint) ->log(); }
449-451: Specify relationship name for activity repeater.Using
nullas the relationship name prevents proper binding to the User model'sactivity()relationship. This should reference the actual relationship name.Apply this diff:
- ->relationship(null, function (Builder $query) { + ->relationship('activity', function (Builder $query) { $query->orderBy('timestamp', 'desc'); })
🧹 Nitpick comments (1)
app/Filament/Admin/Resources/Users/UserResource.php (1)
245-251: Consider adding helper text for external_id field.The
external_idfield would benefit from helper text or a hint explaining its purpose (e.g., for external system integrations or OAuth provider IDs).Example:
TextInput::make('external_id') ->label(trans('admin/user.external_id')) + ->helperText('Identifier from external systems or OAuth providers') ->columnSpan([
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/Filament/Admin/Resources/Users/Pages/EditUser.php(1 hunks)app/Filament/Admin/Resources/Users/UserResource.php(2 hunks)app/Filament/Pages/Auth/EditProfile.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Filament/Admin/Resources/Users/Pages/EditUser.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-07T14:44:18.554Z
Learnt from: notAreYouScared
PR: pelican-dev/panel#1779
File: app/Filament/Admin/Resources/Users/Pages/EditUser.php:51-51
Timestamp: 2025-10-07T14:44:18.554Z
Learning: In the Pelican Panel codebase, when using Filament's FileUpload component for the avatar field in UserResource, the 'avatar' key is intentionally unset from the data array in EditUser::handleRecordUpdate before passing to UserUpdateService. This is by design because the avatar is not stored in the database directly—Filament's FileUpload component handles file storage, retrieval, and deletion through its own lifecycle hooks (formatStateUsing, deleteUploadedFileUsing, etc.) independently of the database update service.
Applied to files:
app/Filament/Pages/Auth/EditProfile.php
🧬 Code graph analysis (1)
app/Filament/Admin/Resources/Users/UserResource.php (9)
app/Extensions/OAuth/OAuthService.php (3)
OAuthService(10-71)get(21-24)unlinkUser(59-70)app/Facades/Activity.php (1)
Activity(8-14)app/Models/ActivityLog.php (4)
ActivityLog(55-265)apiKey(106-109)actor(93-96)htmlable(171-197)app/Models/ApiKey.php (1)
ApiKey(56-234)app/Models/User.php (2)
User(94-492)app/Models/UserSSHKey.php (1)
UserSSHKey(43-70)app/Notifications/MailTested.php (1)
MailTested(9-28)app/Services/Helpers/LanguageService.php (2)
LanguageService(8-30)getAvailableLanguages(22-29)app/Filament/Pages/Auth/EditProfile.php (1)
form(83-533)
🪛 PHPMD (2.15.0)
app/Filament/Admin/Resources/Users/UserResource.php
307-307: Avoid unused local variables such as '$_'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (5)
app/Filament/Pages/Auth/EditProfile.php (1)
154-168: LGTM! Avatar lifecycle handling is correct.The implementation properly manages avatar files through FileUpload's lifecycle hooks without direct database interaction, correctly distinguishing between temporary uploads and persisted disk files.
app/Filament/Admin/Resources/Users/UserResource.php (4)
181-203: LGTM! Mail configuration check is correct.The test mail action now correctly uses
config('mail.default', 'log')to check the Laravel mail driver configuration. The action properly handles exceptions and provides appropriate user feedback.
213-244: LGTM! Password reset implementation is solid.The password reset action correctly integrates with Laravel's Password broker and Filament's reset password flow. Error handling and user notifications are properly implemented.
252-280: LGTM! Timezone and language fields are well-implemented.The timezone and language selects properly use Laravel's
DateTimeZoneand theLanguageService, with appropriate defaults and searchability. The non-native select provides better UX.
281-300: LGTM! Avatar field implementation is correct.The avatar field properly manages the file lifecycle through Filament's FileUpload component, with correct handling of temporary uploads and persisted files. The visibility condition appropriately checks for file existence.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
app/Filament/Admin/Resources/Users/UserResource.php (3)
412-413: Fix null dereference in SSH key delete action.
UserSSHKey::find()returnsnullif the key doesn't exist. Calling->exists()onnullwill throw an error.Apply this diff:
$sshKey = UserSSHKey::find($key['id'] ?? null); -if ($sshKey->exists()) { +if ($sshKey) { $sshKey->delete();
455-456: Fix null relationship name in activity repeater.Using
nullas the relationship name doesn't bind to the User model'sactivityrelation. This prevents the repeater from loading activity records properly.Apply this diff:
-->relationship(null, function (Builder $query) { +->relationship('activity', function (Builder $query) { $query->orderBy('timestamp', 'desc'); })
289-310: Fix variable shadowing, early return, and use refreshed user (resolves PHPMD warning).Three issues in the OAuth unlink logic:
- Variable shadowing: Loop variable
$schemais overwritten at line 290, making code confusing.- Early return: Line 292 exits the entire schema function, preventing subsequent OAuth providers from rendering. Use
continueinstead.- Missing refresh:
unlinkUser()returns a refreshed user (see OAuthService.php), but the code doesn't use the return value before refilling the form.Apply this diff to fix all three issues:
-foreach ($user->oauth as $schema => $_) { - $schema = $oauthService->get($schema); - if (!$schema) { - return; +foreach (array_keys($user->oauth ?? []) as $providerId) { + $provider = $oauthService->get($providerId); + if (!$provider) { + continue; } - $id = $schema->getId(); - $name = $schema->getName(); + $id = $provider->getId(); + $name = $provider->getName(); $actions[] = Action::make("oauth_$id") ->label(trans('profile.unlink', ['name' => $name])) ->icon('tabler-unlink') ->requiresConfirmation() - ->color(Color::hex($schema->getHexColor())) - ->action(function ($livewire) use ($oauthService, $user, $name, $schema) { - $oauthService->unlinkUser($user, $schema); - $livewire->form->fill($user->attributesToArray()); + ->color(Color::hex($provider->getHexColor())) + ->action(function ($livewire) use ($oauthService, $user, $name, $provider) { + $user = $oauthService->unlinkUser($user, $provider); + $livewire->form->fill($user->attributesToArray()); Notification::make() ->title(trans('profile.unlinked', ['name' => $name])) ->success() ->send(); }); }
🧹 Nitpick comments (1)
app/Filament/Admin/Resources/Users/UserResource.php (1)
369-370: Simplify null check in API key delete action.Using
?->exists()is safe but redundant.find()returnsnullif the record doesn't exist, so checkingif ($apiKey)is clearer and more idiomatic.Apply this diff:
$apiKey = ApiKey::find($key['id'] ?? null); -if ($apiKey?->exists()) { +if ($apiKey) { $apiKey->delete();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/Filament/Admin/Resources/Users/UserResource.php(2 hunks)app/Filament/Pages/Auth/EditProfile.php(5 hunks)lang/en/profile.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lang/en/profile.php
- app/Filament/Pages/Auth/EditProfile.php
🧰 Additional context used
🧬 Code graph analysis (1)
app/Filament/Admin/Resources/Users/UserResource.php (9)
app/Extensions/OAuth/OAuthService.php (3)
OAuthService(10-71)get(21-24)unlinkUser(59-70)app/Facades/Activity.php (1)
Activity(8-14)app/Filament/Admin/Resources/Users/Pages/EditUser.php (1)
EditUser(16-55)app/Models/ActivityLog.php (4)
ActivityLog(55-265)apiKey(106-109)actor(93-96)htmlable(171-197)app/Models/ApiKey.php (1)
ApiKey(56-234)app/Models/User.php (4)
User(94-492)apiKeys(242-246)sshKeys(248-251)app/Models/UserSSHKey.php (1)
UserSSHKey(43-70)app/Services/Helpers/LanguageService.php (2)
LanguageService(8-30)getAvailableLanguages(22-29)app/Services/Activity/ActivityLogService.php (3)
event(48-53)property(109-117)log(136-155)
🪛 PHPMD (2.15.0)
app/Filament/Admin/Resources/Users/UserResource.php
289-289: Avoid unused local variables such as '$_'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (2)
app/Filament/Admin/Resources/Users/UserResource.php (2)
191-191: LGTM! Previous review concerns addressed.Excellent improvements addressing past review feedback:
- ✅ Correct mail config usage:
config('mail.default')instead ofconfig('MAIL_MAILER')- ✅ Proper timezone default:
config('app.timezone', 'UTC')- ✅ Avatar field safely handles user creation scenario
- ✅ OAuth unlink actions include confirmation prompts
- ✅ Placeholders added for empty states (OAuth, API keys, SSH keys) for UI consistency
Also applies to: 236-236, 257-257, 300-300, 312-318
329-329: No changes needed—Laravel v12.32.5 supportswhereNot().
Closes #1741
Adds:
Send Test/Reset Password Email actions
Allows admins to set user language/timezone
Allows admins to remove a user's avatar(if set) if they deem it inappropriate
Allows admins to unlink users' sso
Allows admins to ssh/api keys assigned to that user.
Allows admins to see the user activity log.
Before:
After:
