Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughSystematic namespace/import and PHPDoc standardization across the codebase: replaced fully-qualified class names with imports, migrated Carbon imports to Illuminate\Support\Carbon, enriched model docblocks with generics/read-only annotations, adjusted a few method signatures and frontend component parameter names, and bumped Composer dependency versions. No substantive runtime logic changes. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Models/Server.php (1)
511-511:⚠️ Potential issue | 🟡 MinorPotential bug: Bitwise AND instead of logical AND.
Line 511 uses
&(bitwise AND) instead of&&(logical AND). While this may produce the same result in this specific boolean context, it's likely unintentional and should be corrected for clarity.🔧 Proposed fix
- if ($resourceAmount === 0 & $resourceType->isLimit()) { + if ($resourceAmount === 0 && $resourceType->isLimit()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Server.php` at line 511, Change the bitwise AND to a logical AND in the conditional that checks resource amount and limit: replace the use of `&` in the expression involving `$resourceAmount` and `$resourceType->isLimit()` with `&&` so the condition uses logical conjunction (e.g., in the if statement that contains `$resourceAmount` and `$resourceType->isLimit()`).
🧹 Nitpick comments (5)
tests/Integration/Services/Servers/ServerCreationServiceTest.php (1)
60-60: Tighten the$allocationsPHPDoc type.
Allocation::factory()->times(5)->create()returns an Eloquent collection here, notAllocation[]|Collection. This union widens the type and drops the element generic, so PHPStan gets less value from the annotation than it could. Prefer a concrete generic collection type instead.♻️ Proposed fix
-use Illuminate\Database\Eloquent\Collection; +use Illuminate\Database\Eloquent\Collection as EloquentCollection; ... - /** `@var` Allocation[]|Collection $allocations */ + /** `@var` EloquentCollection<int, Allocation> $allocations */ $allocations = Allocation::factory()->times(5)->create([🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Integration/Services/Servers/ServerCreationServiceTest.php` at line 60, Update the PHPDoc for $allocations to a concrete generic Eloquent collection type instead of the loose Allocation[]|Collection union; replace the current doc with something like `@var` \Illuminate\Database\Eloquent\Collection<int, \App\Models\Allocation> $allocations (or import the Eloquent Collection class and use Collection<int, Allocation>) to reflect the result of Allocation::factory()->times(5)->create() and give PHPStan the element type.app/Models/ActivityLog.php (1)
34-34: Keep$propertiesnullable in the PHPDoc.The model still treats
$this->propertiesas nullable in bothwrapProperties()andhasAdditionalMetadata(), so Line 34 currently overstates the guarantee.Suggested change
- * `@property` Collection<array-key, mixed> $properties + * `@property` Collection<array-key, mixed>|null $properties🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ActivityLog.php` at line 34, Update the PHPDoc for the $properties property to indicate it can be null (e.g., Collection<array-key, mixed>|null or ?Collection<array-key, mixed>) because the model code (see wrapProperties() and hasAdditionalMetadata() methods) still treats $this->properties as nullable; change the doc comment on the $properties property in the ActivityLog class to reflect nullable type so the doc matches runtime behavior.tests/Integration/Services/Schedules/ProcessScheduleServiceTest.php (1)
100-101: Fix the inline@vartarget on Line 100.This annotation is attached to
$task2, not$task, so the variable name in the PHPDoc is misleading.Suggested change
- /** `@var` Task $task */ + /** `@var` Task $task2 */ $task2 = Task::factory()->create(['schedule_id' => $schedule->id, 'sequence_id' => 4]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Integration/Services/Schedules/ProcessScheduleServiceTest.php` around lines 100 - 101, The PHPDoc `@var` annotation on the line above Task::factory()->create incorrectly references $task but is meant for $task2; update the inline doc to target $task2 (e.g., change the variable name in the `@var` comment) so the annotation accurately describes the variable created by Task::factory()->create(['schedule_id' => $schedule->id, 'sequence_id' => 4]) and references the Task class.tests/Traits/Integration/CreatesTestModels.php (1)
102-103: Preferrefresh()over asserting awayfresh()here.Line 102 is masking the nullable return from
fresh()right before Line 106 dereferences$model->id.refresh()keeps the persistedEgginstance and avoids the extra PHPDoc assertion.Suggested change
- /** `@var` Egg $model */ - $model = $model->fresh(); + $model->refresh();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Traits/Integration/CreatesTestModels.php` around lines 102 - 103, The test uses $model = $model->fresh() which returns ?Egg and forces a PHPDoc assertion later; replace that with the instance method refresh() to update the existing persisted Egg in place (i.e. call $model->refresh() on the Egg instance) and remove the nullable/extra PHPDoc assertion so the subsequent dereference of $model->id is safe and not masking a nullable return from fresh().app/Models/ActivityLogSubject.php (1)
20-20: Make$subjectnullable in the PHPDoc.
subject()is aMorphTo, so it can resolve tonullwhen the target row no longer exists. The new annotation is stricter than the runtime contract.Suggested change
- * `@property-read` Model|\Eloquent $subject + * `@property-read` Model|\Eloquent|null $subject🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/ActivityLogSubject.php` at line 20, Update the class PHPDoc for ActivityLogSubject to mark the subject property as nullable: change the `@property-read` annotation for $subject (which is returned by the subject() MorphTo relation) to include null (e.g., `@property-read` Model|\Eloquent|null $subject) so the docblock matches the runtime behavior when the related model can be absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Models/Allocation.php`:
- Line 27: Update the PHPDoc for the Allocation model to type the $address
property as string instead of mixed to match the accessor's return type; replace
the `@property-read` mixed $address entry with `@property-read` string $address so
it aligns with the Attribute<string, never> declared in the accessor on the
Allocation class.
In `@app/Models/Backup.php`:
- Line 34: Update the PHPDoc for the Backup model so the `@property-read` for
$status is typed as the BackupStatus enum instead of mixed; locate the Backup
class PHPDoc (the `@property-read` $status entry) and change its type to
BackupStatus to match the enum returned by the getStatusAttribute accessor (the
method around lines 111-115), ensuring static analysis recognizes the enum type.
In `@app/Models/EggVariable.php`:
- Line 23: The `@property` PHPDoc for $rules is incorrectly typed as array<string,
string>; update the doc to reflect that $rules is a list of rule names (e.g.
array<int, string> or string[]) so it matches how validation and
getRequiredAttribute() treat it; locate the $rules property in EggVariable.php
and adjust its PHPDoc accordingly to a string-list type.
In `@app/Models/File.php`:
- Around line 27-29: Update the PHPDoc for the File model to use bool|null for
the three file-type flags instead of int|null: change the `@property` annotations
for is_directory, is_file, and is_symlink to bool|null to match their
declarations in getSchema() and how they are hydrated in getRows(); ensure the
PHPDoc comments near the File class include these exact property names so static
analyzers see the correct types.
In `@app/Models/Schedule.php`:
- Line 35: The PHPDoc for the Schedule model incorrectly types the $status
accessor as mixed; update the docblock to use the ScheduleStatus enum (e.g.
`@property-read` \App\Enums\ScheduleStatus $status) because the accessor on the
Schedule class returns the enum directly; locate the Schedule model PHPDoc (the
`@property-read` $status line) and replace mixed with the ScheduleStatus enum
reference to preserve precise static typing.
In `@app/Models/WebhookConfiguration.php`:
- Line 20: The PHPDoc for the WebhookConfiguration::$events property was widened
to array<array-key,mixed> but the class (methods booted() and updateCache())
treats each element as an event name string; change the docblock back to a
string-list type (e.g. array<int,string> or string[] / list<string>) on the
$events property in the WebhookConfiguration class so callers and static
analysis know each element is a string.
In `@app/Services/Helpers/PluginService.php`:
- Around line 351-354: Remove the stale comment "This throws an error when not
called with qualifier" above the Filament::getPanels() loop in PluginService
(the loop that calls $panel->clearCachedComponents()); it no longer applies
because the unqualified Filament::getPanels() call is valid (as used earlier in
installPlugin()), so delete that comment to avoid confusion and leave the
foreach (Filament::getPanels() as $panel) { $panel->clearCachedComponents(); }
block unchanged.
In `@public/js/filament/schemas/components/wizard.js`:
- Line 1: The init() in the exported p() component always seeds this.step from
startStep (param o) causing query-string persistence
(isStepPersistedInQueryString / n) to be ignored; change init() so when n is
true it reads the step value from the URL using stepQueryStringKey (h),
validates it against getSteps(), and uses that value (falling back to startStep
if missing/invalid), then proceed to set up the $watch for "step", call
autofocusFields(), and keep updateQueryString() as-is so reloads/deep-links
respect the URL; update the logic in the init function inside p to prefer the
query string when present and valid.
In `@tests/Assertions/AssertsActivityLogged.php`:
- Around line 13-16: The PHPDoc for assertActivityFor is out of sync: it
documents a single $subjects parameter but the method signature uses a variadic
...$subjects. Update the docblock to reflect the variadic parameter (e.g.,
document as `@param` Model|array ...$subjects or `@param` (Model|array)[] $subjects
variadic) so it matches the function signature for assertActivityFor.
In `@tests/Integration/Services/Servers/BuildModificationServiceTest.php`:
- Around line 37-38: The docblock for $allocations is incorrect: replace the
array type with the Eloquent collection type returned by
Allocation::factory()->times(4)->create(...) by annotating $allocations as
Collection<int, Allocation> (or \Illuminate\Database\Eloquent\Collection<int,
Allocation>) so static analyzers and IDEs understand that $allocations is a
collection rather than an array; update the docblock above the $allocations
variable accordingly.
---
Outside diff comments:
In `@app/Models/Server.php`:
- Line 511: Change the bitwise AND to a logical AND in the conditional that
checks resource amount and limit: replace the use of `&` in the expression
involving `$resourceAmount` and `$resourceType->isLimit()` with `&&` so the
condition uses logical conjunction (e.g., in the if statement that contains
`$resourceAmount` and `$resourceType->isLimit()`).
---
Nitpick comments:
In `@app/Models/ActivityLog.php`:
- Line 34: Update the PHPDoc for the $properties property to indicate it can be
null (e.g., Collection<array-key, mixed>|null or ?Collection<array-key, mixed>)
because the model code (see wrapProperties() and hasAdditionalMetadata()
methods) still treats $this->properties as nullable; change the doc comment on
the $properties property in the ActivityLog class to reflect nullable type so
the doc matches runtime behavior.
In `@app/Models/ActivityLogSubject.php`:
- Line 20: Update the class PHPDoc for ActivityLogSubject to mark the subject
property as nullable: change the `@property-read` annotation for $subject (which
is returned by the subject() MorphTo relation) to include null (e.g.,
`@property-read` Model|\Eloquent|null $subject) so the docblock matches the
runtime behavior when the related model can be absent.
In `@tests/Integration/Services/Schedules/ProcessScheduleServiceTest.php`:
- Around line 100-101: The PHPDoc `@var` annotation on the line above
Task::factory()->create incorrectly references $task but is meant for $task2;
update the inline doc to target $task2 (e.g., change the variable name in the
`@var` comment) so the annotation accurately describes the variable created by
Task::factory()->create(['schedule_id' => $schedule->id, 'sequence_id' => 4])
and references the Task class.
In `@tests/Integration/Services/Servers/ServerCreationServiceTest.php`:
- Line 60: Update the PHPDoc for $allocations to a concrete generic Eloquent
collection type instead of the loose Allocation[]|Collection union; replace the
current doc with something like `@var`
\Illuminate\Database\Eloquent\Collection<int, \App\Models\Allocation>
$allocations (or import the Eloquent Collection class and use Collection<int,
Allocation>) to reflect the result of Allocation::factory()->times(5)->create()
and give PHPStan the element type.
In `@tests/Traits/Integration/CreatesTestModels.php`:
- Around line 102-103: The test uses $model = $model->fresh() which returns ?Egg
and forces a PHPDoc assertion later; replace that with the instance method
refresh() to update the existing persisted Egg in place (i.e. call
$model->refresh() on the Egg instance) and remove the nullable/extra PHPDoc
assertion so the subsequent dereference of $model->id is safe and not masking a
nullable return from fresh().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1aaed3c2-d591-4a8b-9a85-a454cc82c1ca
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (95)
app/Filament/Admin/Pages/ListLogs.phpapp/Filament/Admin/Resources/Nodes/Pages/EditNode.phpapp/Filament/Admin/Resources/Servers/Pages/EditServer.phpapp/Filament/Server/Pages/Settings.phpapp/Filament/Server/Resources/Activities/ActivityResource.phpapp/Filament/Server/Resources/Files/Pages/EditFiles.phpapp/Http/Middleware/RequireTwoFactorAuthentication.phpapp/Http/Requests/Api/Application/ApplicationApiRequest.phpapp/Livewire/NodeClientConnectivity.phpapp/Models/ActivityLog.phpapp/Models/ActivityLogSubject.phpapp/Models/Allocation.phpapp/Models/ApiKey.phpapp/Models/Backup.phpapp/Models/Database.phpapp/Models/DatabaseHost.phpapp/Models/Egg.phpapp/Models/EggVariable.phpapp/Models/File.phpapp/Models/Mount.phpapp/Models/Node.phpapp/Models/NodeRole.phpapp/Models/Plugin.phpapp/Models/Role.phpapp/Models/Schedule.phpapp/Models/Server.phpapp/Models/ServerTransfer.phpapp/Models/ServerVariable.phpapp/Models/Subuser.phpapp/Models/Task.phpapp/Models/Traits/HasAccessTokens.phpapp/Models/User.phpapp/Models/UserSSHKey.phpapp/Models/Webhook.phpapp/Models/WebhookConfiguration.phpapp/Providers/AppServiceProvider.phpapp/Services/Databases/DatabaseManagementService.phpapp/Services/Helpers/PluginService.phpapp/helpers.phpbootstrap/app.phpbootstrap/providers.phpcomposer.jsonconfig/database.phpconfig/fractal.phpconfig/health.phpconfig/permission.phpconfig/sanctum.phpdatabase/migrations/2018_01_13_142012_SetupTableForKeyEncryption.phpdatabase/migrations/2018_02_25_160152_remove_default_null_value_on_table.phpdatabase/migrations/2020_04_10_141024_store_node_tokens_as_encrypted_value.phpdatabase/migrations/2023_01_24_210051_add_uuid_column_to_failed_jobs_table.phpdatabase/migrations/2024_04_21_162552_create_webhooks_table.phpdatabase/migrations/2024_07_19_130942_create_permission_tables.phpphpstan.neonpublic/css/filament/filament/app.csspublic/js/filament/filament/app.jspublic/js/filament/forms/components/color-picker.jspublic/js/filament/forms/components/select.jspublic/js/filament/notifications/notifications.jspublic/js/filament/schemas/components/tabs.jspublic/js/filament/schemas/components/wizard.jspublic/js/filament/support/support.jspublic/js/filament/tables/components/columns/select.jstests/Assertions/AssertsActivityLogged.phptests/Feature/SettingsControllerTest.phptests/Integration/Api/Client/AccountControllerTest.phptests/Integration/Api/Client/ApiKeyControllerTest.phptests/Integration/Api/Client/ClientControllerTest.phptests/Integration/Api/Client/Server/Allocation/CreateNewAllocationTest.phptests/Integration/Api/Client/Server/Allocation/DeleteAllocationTest.phptests/Integration/Api/Client/Server/Backup/DeleteBackupTest.phptests/Integration/Api/Client/Server/Schedule/CreateServerScheduleTest.phptests/Integration/Api/Client/Server/Schedule/ExecuteScheduleTest.phptests/Integration/Api/Client/Server/Schedule/GetServerSchedulesTest.phptests/Integration/Api/Client/Server/Schedule/UpdateServerScheduleTest.phptests/Integration/Api/Client/Server/ScheduleTask/CreateServerScheduleTaskTest.phptests/Integration/Api/Client/Server/SettingsControllerTest.phptests/Integration/Api/Client/Server/Startup/GetStartupAndVariablesTest.phptests/Integration/Api/Client/Server/Startup/UpdateStartupVariableTest.phptests/Integration/Api/Client/Server/Subuser/CreateServerSubuserTest.phptests/Integration/Api/Client/Server/Subuser/DeleteSubuserTest.phptests/Integration/Api/Client/Server/Subuser/SubuserAuthorizationTest.phptests/Integration/Api/Client/Server/Subuser/UpdateSubuserTest.phptests/Integration/Api/Client/Server/WebsocketControllerTest.phptests/Integration/Api/Daemon/DaemonAuthenticateTest.phptests/Integration/Jobs/Schedule/RunTaskJobTest.phptests/Integration/Services/Schedules/ProcessScheduleServiceTest.phptests/Integration/Services/Servers/BuildModificationServiceTest.phptests/Integration/Services/Servers/ServerCreationServiceTest.phptests/Integration/Services/Servers/ServerDeletionServiceTest.phptests/Integration/Services/Servers/StartupModificationServiceTest.phptests/Pest.phptests/Traits/Http/MocksMiddlewareClosure.phptests/Traits/Integration/CreatesTestModels.phptests/Traits/MocksPdoConnection.php
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/Models/File.php (1)
27-29:⚠️ Potential issue | 🟡 MinorKeep these file-type flags documented as booleans.
Lines 27-29 still regress
is_directory,is_file, andis_symlinktoint, butgetSchema()declares them asbooleanand the rest of the codebase uses them as boolean flags. This leaves the PHPDoc misleading for static analysis and IDE help.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/File.php` around lines 27 - 29, Update the PHPDoc for the File model so the file-type flags are documented as booleans: change the PHPDoc types for is_directory, is_file, and is_symlink from int to bool to match getSchema() and how the rest of the codebase uses them; locate the docblock containing those `@property` annotations in the File class and replace the int types with boolean (or bool) to keep static analysis and IDE hints correct.
🧹 Nitpick comments (1)
app/Models/Backup.php (1)
22-22: Consider narrowing$ignored_filestype for better static analysis.The
ignored_filesproperty is used withimplode("\n", $backup->ignored_files)inDaemonBackupRepository, which expects string elements. The widened typearray<array-key, mixed>loses this guarantee.✏️ Suggested type narrowing
- * `@property` array<array-key, mixed> $ignored_files + * `@property` string[] $ignored_files🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Backup.php` at line 22, The property docblock for Backup::$ignored_files is too broad; change its type from array<array-key, mixed> to a string-array (e.g. array<int, string> or string[] ) so static analysis reflects that implode("\n", $backup->ignored_files) yields strings; update the `@property` annotation in the Backup class to "@property array<int,string>|string[] $ignored_files" (and if necessary ensure the Eloquent cast for 'ignored_files' is 'array' so values are always strings).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/Models/File.php`:
- Around line 27-29: Update the PHPDoc for the File model so the file-type flags
are documented as booleans: change the PHPDoc types for is_directory, is_file,
and is_symlink from int to bool to match getSchema() and how the rest of the
codebase uses them; locate the docblock containing those `@property` annotations
in the File class and replace the int types with boolean (or bool) to keep
static analysis and IDE hints correct.
---
Nitpick comments:
In `@app/Models/Backup.php`:
- Line 22: The property docblock for Backup::$ignored_files is too broad; change
its type from array<array-key, mixed> to a string-array (e.g. array<int, string>
or string[] ) so static analysis reflects that implode("\n",
$backup->ignored_files) yields strings; update the `@property` annotation in the
Backup class to "@property array<int,string>|string[] $ignored_files" (and if
necessary ensure the Eloquent cast for 'ignored_files' is 'array' so values are
always strings).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 10e34099-a986-43a8-a777-be1ded89b826
📒 Files selected for processing (9)
app/Models/Allocation.phpapp/Models/Backup.phpapp/Models/Database.phpapp/Models/EggVariable.phpapp/Models/File.phpapp/Models/Schedule.phpapp/Models/Server.phpapp/Models/WebhookConfiguration.phpapp/Services/Helpers/PluginService.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Services/Helpers/PluginService.php
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Supersedes #2274