Collection of smaller v4 fixes #1684
Conversation
can't use old secret in new system
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 WalkthroughWalkthroughRedirect query key for server power actions changed from Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser
participant ListServersPage
participant ServerService
Note right of ListServersPage #DDEEF8: User triggers server power action
Browser->>ListServersPage: request power action
ListServersPage->>ServerService: perform action (start/restart/stop/kill)
ServerService-->>ListServersPage: success / exception
alt success
ListServersPage->>Browser: redirect to /servers?tab=...
else error
ListServersPage->>Browser: show error notification
end
sequenceDiagram
autonumber
participant AdminUI
participant ScheduleResource
Note right of ScheduleResource #F6F0F8: Resolve enum-backed schedule status for UI
AdminUI->>ScheduleResource: formatStateUsing(?Schedule $schedule)
alt $schedule is null
ScheduleResource-->>AdminUI: return default ('new' / inactive)
else $schedule exists
ScheduleResource-->>AdminUI: return $schedule->status->value (inactive/processing/active)
end
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning, 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: notCharles <charles@pelican.dev>
Co-authored-by: notCharles <charles@pelican.dev>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
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)
545-559: Bug: console_graph_period not unset; may hit mass-assignment/unknown column on saveYou add console_graph_period to customization but don’t unset it from $data. Also, persist proper types when saving.
Apply this diff:
- $customization = [ - 'console_font' => $data['console_font'], - 'console_font_size' => $data['console_font_size'], - 'console_rows' => $data['console_rows'], - 'console_graph_period' => $data['console_graph_period'], - 'dashboard_layout' => $data['dashboard_layout'], - 'top_navigation' => $data['top_navigation'], - ]; + $customization = [ + 'console_font' => (string) $data['console_font'], + 'console_font_size' => (int) $data['console_font_size'], + 'console_rows' => (int) $data['console_rows'], + 'console_graph_period' => (int) $data['console_graph_period'], + 'dashboard_layout' => (string) $data['dashboard_layout'], + 'top_navigation' => (bool) $data['top_navigation'], + ]; @@ - unset($data['console_font'],$data['console_font_size'], $data['console_rows'], $data['dashboard_layout'], $data['top_navigation']); + unset( + $data['console_font'], + $data['console_font_size'], + $data['console_rows'], + $data['console_graph_period'], + $data['dashboard_layout'], + $data['top_navigation'], + );
♻️ Duplicate comments (1)
app/Filament/Server/Resources/Files/Pages/ListFiles.php (1)
86-95: deferLoading is ineffective here — move File::get into ->query and cache with once()
$files = File::get(...)runs eagerly, so IO still happens before the table defers. Cache the call per request while truly deferring the fetch.- $files = File::get($server, $this->path); ... - ->query(fn () => $files->orderByDesc('is_directory')) + ->query(fn () => once(fn () => File::get($server, $this->path))->orderByDesc('is_directory'))
🧹 Nitpick comments (8)
app/Filament/Pages/Auth/Login.php (1)
37-40: Guard against undefined property access when hiding captcha.If userUndertakingMultiFactorAuthentication isn’t always defined, this can emit notices in PHP 8.2+. Add a property_exists check.
Apply:
- $components[] = $captchaComponent - ->hidden(fn () => filled($this->userUndertakingMultiFactorAuthentication)); + $components[] = $captchaComponent + ->hidden(fn () => property_exists($this, 'userUndertakingMultiFactorAuthentication') + && filled($this->userUndertakingMultiFactorAuthentication));app/Filament/Pages/Auth/EditProfile.php (1)
149-156: Avatar on public disk—add limitsStoring on disk('public') is fine. Add a size cap to prevent large uploads.
Apply this diff:
- ->acceptedFileTypes(['image/png']) + ->acceptedFileTypes(['image/png']) ->directory('avatars') ->disk('public') + ->maxSize(1024) // 1 MBAlso verify storage:link is in deploy steps so /storage/avatars/{id}.png resolves publicly.
public/js/filament/forms/components/select.js (3)
1-1: Clear active descendant on close to avoid stale a11y reference
closeDropdown()removes selection classes but keepsaria-activedescendanton the listbox. Clear it on close.closeDropdown(){ this.dropdown.style.display="none", this.selectButton.setAttribute("aria-expanded","false"), this.isOpen=!1, + this.dropdown.removeAttribute("aria-activedescendant"), this.resizeListener&&(window.removeEventListener("resize",this.resizeListener),this.resizeListener=null), ... }
1-1: Value type mismatch can prevent selection highlightComparisons use strict equality between
data-value(string) andstate(may be number), e.g.,e[s].getAttribute("data-value")===this.state. Normalize to string when comparing to avoid missed highlights/selection.Example adjustments in the relevant spots:
- if(e[s].getAttribute("data-value")===this.state){ + if(String(e[s].getAttribute("data-value"))===String(this.state)){ - if(this.state.includes(e[s].getAttribute("data-value"))){ + if(this.state.map(String).includes(String(e[s].getAttribute("data-value")))){Please confirm whether
statecan be numeric in your usage; if yes, the normalization prevents subtle UI desyncs.
1-1: i18n: hardcoded “Search” aria-label
this.searchInput.setAttribute("aria-label","Search")is hardcoded English. Prefer using a prop (e.g.,searchAriaLabel) or reusesearchPrompt(sans ellipsis) for localization.- this.searchInput.setAttribute("aria-label","Search"), + this.searchInput.setAttribute("aria-label",(this.searchAriaLabel ?? (this.searchPrompt || "")).toString().replace(/\u2026|\.\.\.$/,"")),public/js/filament/tables/components/columns/select.js (3)
1-1: Clear active descendant on close to avoid stale a11y referenceMirror the
aria-activedescendantcleanup incloseDropdown()here as well.closeDropdown(){ this.dropdown.style.display="none", this.selectButton.setAttribute("aria-expanded","false"), this.isOpen=!1, + this.dropdown.removeAttribute("aria-activedescendant"), ... }
1-1: Value type mismatch can prevent selection highlightNormalize comparisons between
data-valueandstateto strings in this build too (same rationale and snippets as the forms component).
1-1: i18n: hardcoded “Search” aria-labelReplace the hardcoded
"Search"with a localized prop or reusesearchPromptconsistently as an accessible label.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
app/Filament/Admin/Resources/Nodes/RelationManagers/AllocationsRelationManager.php(1 hunks)app/Filament/Pages/Auth/EditProfile.php(8 hunks)app/Filament/Pages/Auth/Login.php(2 hunks)app/Filament/Server/Resources/Files/Pages/ListFiles.php(3 hunks)app/Providers/AppServiceProvider.php(1 hunks)public/js/filament/filament/app.js(1 hunks)public/js/filament/forms/components/select.js(1 hunks)public/js/filament/forms/components/slider.js(1 hunks)public/js/filament/tables/components/columns/select.js(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- public/js/filament/forms/components/slider.js
- public/js/filament/filament/app.js
🧰 Additional context used
🧬 Code graph analysis (3)
app/Filament/Pages/Auth/Login.php (8)
app/Extensions/OAuth/OAuthSchemaInterface.php (1)
getHexColor(32-32)app/Extensions/OAuth/Schemas/OAuthSchema.php (1)
getHexColor(112-115)app/Extensions/OAuth/Schemas/AuthentikSchema.php (1)
getHexColor(59-62)app/Extensions/OAuth/Schemas/CommonSchema.php (1)
getHexColor(35-38)app/Extensions/OAuth/Schemas/DiscordSchema.php (1)
getHexColor(50-53)app/Extensions/OAuth/Schemas/GithubSchema.php (1)
getHexColor(50-53)app/Extensions/OAuth/Schemas/GitlabSchema.php (1)
getHexColor(61-64)app/Extensions/OAuth/Schemas/SteamSchema.php (1)
getHexColor(67-70)
app/Filament/Server/Resources/Files/Pages/ListFiles.php (1)
app/Services/Activity/ActivityLogService.php (1)
property(109-117)
app/Filament/Pages/Auth/EditProfile.php (8)
app/Extensions/OAuth/OAuthSchemaInterface.php (1)
getHexColor(32-32)app/Extensions/OAuth/Schemas/AuthentikSchema.php (1)
getHexColor(59-62)app/Extensions/OAuth/Schemas/CommonSchema.php (1)
getHexColor(35-38)app/Extensions/OAuth/Schemas/DiscordSchema.php (1)
getHexColor(50-53)app/Extensions/OAuth/Schemas/GithubSchema.php (1)
getHexColor(50-53)app/Extensions/OAuth/Schemas/GitlabSchema.php (1)
getHexColor(61-64)app/Extensions/OAuth/Schemas/SteamSchema.php (1)
getHexColor(67-70)app/Models/User.php (1)
getCustomization(323-329)
🔇 Additional comments (11)
app/Filament/Admin/Resources/Nodes/RelationManagers/AllocationsRelationManager.php (1)
45-45: Pagination options should be ints — good fixSwitching to integer page sizes aligns with Filament expectations.
app/Filament/Server/Resources/Files/Pages/ListFiles.php (2)
337-337: Label hidden for delete — consistent with UI tweaksMatches the pattern used elsewhere in the PR.
436-438: Activity “new_file” property now uses the resolved path — goodUsing
$pathavoids double-joining and keeps analytics consistent.Confirm downstream consumers expect the key to be
file(notpath) for “server:file.write”.app/Filament/Pages/Auth/Login.php (1)
79-84: Null‑safety for OAuth button color — set color only when hex provided.getHexColor() can be null; calling Color::hex(null) will error. Construct the Action, then call ->color(...) only if a hex string exists.
File: app/Filament/Pages/Auth/Login.php — lines ~79–84
- $actions[] = Action::make("oauth_$id") - ->label($schema->getName()) - ->icon($schema->getIcon()) - ->color(Color::hex($schema->getHexColor())) - ->url(route('auth.oauth.redirect', ['driver' => $id], false)); + $action = Action::make("oauth_$id") + ->label($schema->getName()) + ->icon($schema->getIcon()) + ->url(route('auth.oauth.redirect', ['driver' => $id], false)); + if ($hex = $schema->getHexColor()) { + $action->color(Color::hex($hex)); + } + $actions[] = $action;Verification scripts returned no files in the sandbox; confirm in-repo and apply the patch.
app/Providers/AppServiceProvider.php (1)
107-115: Confirm 'blurple' shade usage & Color::hex behaviorrg reported "No files were searched" — I couldn't verify whether any shade-specific classes (e.g., blurple-500) exist or whether Color::hex('#5865F2') generates a full shade palette in your Filament version.
- Action: Search the repo for shade-specific uses and paste results, e.g. run locally:
git grep -nE 'blurple(-[0-9]{2,3})?' || rg --hidden --no-ignore -n -e '\bblurple-(50|100|200|300|400|500|600|700|800|900|950)\b' -e '\bblurple\b'- Action: Confirm your Filament version and whether Color::hex produces multi‑shade tokens (check Filament docs or inspect FilamentColor::register output).
File: app/Providers/AppServiceProvider.php (lines 107–115).
app/Filament/Pages/Auth/EditProfile.php (6)
181-181: Verify translation keys existEnsure profile.link and profile.unlink exist and include the {name} placeholder to avoid runtime “missing translation” strings.
237-237: Confirm tab query valuesDouble-check that api-keys::data::tab and ssh-keys::data::tab match the actual persisted tab key Filament expects. If unsure, set explicit IDs on the tabs (e.g., ->id('api-keys') / ->id('ssh-keys')) and use those.
Also applies to: 320-320
428-432: BooleanStateCast mapping—verify behaviorOptions are 1/0 while you cast to bool via new BooleanStateCast(false, true). Confirm this maps selected values to true/false as intended and that persistence round-trips correctly.
506-506: hintIcon tooltip usage looks goodMigration to the new two-argument signature is correct here.
566-571: Good: coercing types on fillCasting numeric and boolean preferences on fill is correct. Paired with the save-side casting in the earlier comment.
30-30: Import OKBooleanStateCast import aligns with its usage below.
| ->label(trans('profile.' . ($unlink ? 'unlink' : 'link'), ['name' => $name])) | ||
| ->icon($unlink ? 'tabler-unlink' : 'tabler-link') | ||
| ->color(Color::generateV3Palette($schema->getHexColor())) | ||
| ->color(Color::hex($schema->getHexColor())) | ||
| ->action(function (UserUpdateService $updateService) use ($id, $name, $unlink) { |
There was a problem hiding this comment.
Null-safe OAuth action color
OAuthSchemaInterface::getHexColor() is nullable; Color::hex(null) will fail for schemas that don’t define a color.
Apply this diff:
- ->color(Color::hex($schema->getHexColor()))
+ ->color(fn () => Color::hex($schema->getHexColor() ?? '#64748B'))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ->label(trans('profile.' . ($unlink ? 'unlink' : 'link'), ['name' => $name])) | |
| ->icon($unlink ? 'tabler-unlink' : 'tabler-link') | |
| ->color(Color::generateV3Palette($schema->getHexColor())) | |
| ->color(Color::hex($schema->getHexColor())) | |
| ->action(function (UserUpdateService $updateService) use ($id, $name, $unlink) { | |
| ->label(trans('profile.' . ($unlink ? 'unlink' : 'link'), ['name' => $name])) | |
| ->icon($unlink ? 'tabler-unlink' : 'tabler-link') | |
| ->color(fn () => Color::hex($schema->getHexColor() ?? '#64748B')) | |
| ->action(function (UserUpdateService $updateService) use ($id, $name, $unlink) { |
🤖 Prompt for AI Agents
In app/Filament/Pages/Auth/EditProfile.php around lines 181 to 184,
Color::hex($schema->getHexColor()) can receive null because
OAuthSchemaInterface::getHexColor() is nullable; change the code to handle null
safely by using a fallback or conditional: if getHexColor() returns a string use
Color::hex(...) otherwise pass a sensible default color or omit the color call
so no exception is thrown. Ensure the fix uses the schema hex only when non-null
(e.g. null-coalesce to a default hex or wrap the Color::hex call in a
conditional) so null values don’t cause failures.
See commit history for all changes.
Notable changes: