Refactor schedule tasks #1911
Refactor schedule tasks #1911
Conversation
📝 WalkthroughWalkthroughIntroduces a pluggable task schema system: interface and abstract base class, four concrete task schemas (power, command, backup, delete-files), TaskService with provider registration, dynamic Filament UI wiring in the Schedules Tasks relation manager, RunTaskJob delegation to TaskService, and Task model schema resolution. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as TasksRelationManager
participant TS as TaskService
participant Schema as TaskSchema
participant Job as RunTaskJob
participant Server as Server/Daemon
UI->>TS: getAll()
TS-->>UI: schemas (ids, names, payload forms, defaults)
UI->>UI: build action select & payload form from schema
UI->>Job: enqueue RunTaskJob(task)
rect `#E6F5E6`
Job->>TS: get(task.action)
TS-->>Job: schema or null
alt schema found
Job->>Schema: runTask(task)
Schema->>Server: perform specific action (power/command/backup/delete)
else missing
Job-->>Job: throw InvalidArgumentException
end
end
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Jobs/Schedule/RunTaskJob.php (1)
31-65: Add null safety check for unregistered task actions.Line 54 chains
$taskService->get($this->task->action)->runTask($this->task)without checking if the schema exists. IfTaskService::get()returnsnull(e.g., for legacy actions or after a custom schema is unregistered), this will cause a fatal error.Apply this diff to add defensive null checking:
// Perform the provided task against the daemon. try { - $taskService->get($this->task->action)->runTask($this->task); + $schema = $taskService->get($this->task->action); + if (!$schema) { + throw new \RuntimeException("Unknown task action: {$this->task->action}"); + } + + $schema->runTask($this->task); } catch (Exception $exception) {
🧹 Nitpick comments (1)
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (1)
71-72: Consider dependency injection for TaskService.While using
app()is functional, injecting TaskService via the constructor would be more testable and align with Laravel best practices.Example refactor:
+public function __construct(private TaskService $taskService) +{ + parent::__construct(); +} + public function table(Table $table): Table { /** @var Schedule $schedule */ $schedule = $this->getOwnerRecord(); - // @phpstan-ignore myCustomRules.forbiddenGlobalFunctions - $tasks = app(TaskService::class)->getAll(); + $tasks = $this->taskService->getAll();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app/Extensions/Tasks/Schemas/CreateBackupSchema.php(1 hunks)app/Extensions/Tasks/Schemas/DeleteFilesSchema.php(1 hunks)app/Extensions/Tasks/Schemas/PowerActionSchema.php(1 hunks)app/Extensions/Tasks/Schemas/SendCommandSchema.php(1 hunks)app/Extensions/Tasks/Schemas/TaskSchema.php(1 hunks)app/Extensions/Tasks/TaskSchemaInterface.php(1 hunks)app/Extensions/Tasks/TaskService.php(1 hunks)app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php(4 hunks)app/Jobs/Schedule/RunTaskJob.php(3 hunks)app/Providers/Extensions/TaskServiceProvider.php(1 hunks)bootstrap/providers.php(1 hunks)lang/en/server/schedule.php(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-15T11:55:53.461Z
Learnt from: rmartinoscar
Repo: pelican-dev/panel PR: 1801
File: app/Extensions/OAuth/Schemas/AuthentikSchema.php:7-10
Timestamp: 2025-10-15T11:55:53.461Z
Learning: In Filament v4, Wizard Step components use the Filament\Schemas namespace (Filament\Schemas\Components\Wizard\Step), not Filament\Forms.
Applied to files:
app/Extensions/Tasks/Schemas/SendCommandSchema.php
🧬 Code graph analysis (11)
app/Providers/Extensions/TaskServiceProvider.php (8)
app/Extensions/Tasks/Schemas/CreateBackupSchema.php (1)
CreateBackupSchema(9-32)app/Extensions/Tasks/Schemas/DeleteFilesSchema.php (1)
DeleteFilesSchema(8-26)app/Extensions/Tasks/Schemas/PowerActionSchema.php (1)
PowerActionSchema(11-57)app/Extensions/Tasks/Schemas/SendCommandSchema.php (1)
SendCommandSchema(9-36)app/Extensions/Tasks/TaskService.php (2)
TaskService(5-31)register(23-30)app/Repositories/Daemon/DaemonServerRepository.php (1)
DaemonServerRepository(13-165)app/Services/Backups/InitiateBackupService.php (1)
InitiateBackupService(16-131)app/Services/Files/DeleteFilesService.php (1)
DeleteFilesService(10-40)
bootstrap/providers.php (1)
app/Providers/Extensions/TaskServiceProvider.php (1)
TaskServiceProvider(15-31)
app/Extensions/Tasks/Schemas/TaskSchema.php (5)
app/Extensions/Tasks/TaskSchemaInterface.php (7)
getName(13-13)getId(11-11)canCreate(17-17)getDefaultPayload(19-19)getPayloadLabel(21-21)formatPayload(24-24)getPayloadForm(27-27)app/Extensions/Tasks/Schemas/CreateBackupSchema.php (3)
getId(13-16)canCreate(23-26)getPayloadLabel(28-31)app/Extensions/Tasks/Schemas/DeleteFilesSchema.php (2)
getId(12-15)getPayloadLabel(22-25)app/Extensions/Tasks/Schemas/PowerActionSchema.php (5)
getId(15-18)getDefaultPayload(25-28)getPayloadLabel(30-33)formatPayload(35-38)getPayloadForm(41-56)app/Extensions/Tasks/Schemas/SendCommandSchema.php (3)
getId(11-14)getPayloadLabel(21-24)getPayloadForm(27-35)
app/Extensions/Tasks/Schemas/CreateBackupSchema.php (6)
app/Models/Schedule.php (1)
Schedule(38-145)app/Models/Task.php (1)
Task(27-123)app/Services/Backups/InitiateBackupService.php (2)
InitiateBackupService(16-131)setIgnoredFiles(49-65)app/Extensions/Tasks/Schemas/TaskSchema.php (3)
TaskSchema(10-52)canCreate(17-20)getPayloadLabel(27-30)app/Extensions/Tasks/Schemas/DeleteFilesSchema.php (4)
__construct(10-10)getId(12-15)runTask(17-20)getPayloadLabel(22-25)app/Jobs/Schedule/RunTaskJob.php (2)
__construct(24-24)handle(31-65)
app/Extensions/Tasks/Schemas/PowerActionSchema.php (4)
app/Models/Task.php (1)
Task(27-123)app/Repositories/Daemon/DaemonServerRepository.php (2)
DaemonServerRepository(13-165)power(159-164)app/Extensions/Tasks/Schemas/TaskSchema.php (5)
TaskSchema(10-52)getDefaultPayload(22-25)getPayloadLabel(27-30)formatPayload(33-40)getPayloadForm(43-51)app/Repositories/Daemon/DaemonRepository.php (1)
setServer(24-31)
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (9)
app/Extensions/Tasks/TaskService.php (3)
TaskService(5-31)get(18-21)getAll(13-16)app/Models/Schedule.php (2)
Schedule(38-145)tasks(133-136)app/Models/Task.php (1)
Task(27-123)app/Extensions/Tasks/Schemas/CreateBackupSchema.php (3)
canCreate(23-26)getId(13-16)getPayloadLabel(28-31)app/Extensions/Tasks/Schemas/TaskSchema.php (6)
canCreate(17-20)getName(12-15)getDefaultPayload(22-25)getPayloadForm(43-51)formatPayload(33-40)getPayloadLabel(27-30)app/Extensions/Tasks/TaskSchemaInterface.php (7)
canCreate(17-17)getId(11-11)getName(13-13)getDefaultPayload(19-19)getPayloadForm(27-27)formatPayload(24-24)getPayloadLabel(21-21)app/Extensions/Tasks/Schemas/DeleteFilesSchema.php (2)
getId(12-15)getPayloadLabel(22-25)app/Extensions/Tasks/Schemas/PowerActionSchema.php (5)
getId(15-18)getDefaultPayload(25-28)getPayloadForm(41-56)formatPayload(35-38)getPayloadLabel(30-33)app/Extensions/Tasks/Schemas/SendCommandSchema.php (3)
getId(11-14)getPayloadForm(27-35)getPayloadLabel(21-24)
app/Extensions/Tasks/Schemas/SendCommandSchema.php (3)
app/Models/Task.php (1)
Task(27-123)app/Extensions/Tasks/Schemas/TaskSchema.php (4)
TaskSchema(10-52)getPayloadLabel(27-30)getPayloadForm(43-51)getDefaultPayload(22-25)app/Extensions/Tasks/TaskSchemaInterface.php (5)
getId(11-11)runTask(15-15)getPayloadLabel(21-21)getPayloadForm(27-27)getDefaultPayload(19-19)
app/Extensions/Tasks/TaskSchemaInterface.php (3)
app/Models/Schedule.php (1)
Schedule(38-145)app/Models/Task.php (1)
Task(27-123)app/Extensions/Tasks/Schemas/TaskSchema.php (6)
getName(12-15)canCreate(17-20)getDefaultPayload(22-25)getPayloadLabel(27-30)formatPayload(33-40)getPayloadForm(43-51)
app/Extensions/Tasks/TaskService.php (6)
app/Providers/Extensions/TaskServiceProvider.php (1)
register(17-30)app/Extensions/Tasks/Schemas/CreateBackupSchema.php (1)
getId(13-16)app/Extensions/Tasks/Schemas/DeleteFilesSchema.php (1)
getId(12-15)app/Extensions/Tasks/Schemas/PowerActionSchema.php (1)
getId(15-18)app/Extensions/Tasks/Schemas/SendCommandSchema.php (1)
getId(11-14)app/Extensions/Tasks/TaskSchemaInterface.php (1)
getId(11-11)
app/Extensions/Tasks/Schemas/DeleteFilesSchema.php (4)
app/Models/Task.php (1)
Task(27-123)app/Services/Files/DeleteFilesService.php (1)
DeleteFilesService(10-40)app/Extensions/Tasks/Schemas/TaskSchema.php (2)
TaskSchema(10-52)getPayloadLabel(27-30)app/Jobs/Schedule/RunTaskJob.php (2)
__construct(24-24)handle(31-65)
app/Jobs/Schedule/RunTaskJob.php (6)
app/Extensions/Tasks/TaskService.php (2)
TaskService(5-31)get(18-21)app/Extensions/Tasks/Schemas/CreateBackupSchema.php (1)
runTask(18-21)app/Extensions/Tasks/Schemas/DeleteFilesSchema.php (1)
runTask(17-20)app/Extensions/Tasks/Schemas/PowerActionSchema.php (1)
runTask(20-23)app/Extensions/Tasks/Schemas/SendCommandSchema.php (1)
runTask(16-19)app/Extensions/Tasks/TaskSchemaInterface.php (1)
runTask(15-15)
🪛 PHPMD (2.15.0)
app/Extensions/Tasks/Schemas/TaskSchema.php
17-17: Avoid unused parameters such as '$schedule'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (14)
lang/en/server/schedule.php (1)
79-79: LGTM!The translation updates align with the task schema refactoring: adding
no_payloadfor tasks without payload requirements and renamingdeletetodelete_filesfor clarity and consistency withTask::ACTION_DELETE_FILES.Also applies to: 103-103
bootstrap/providers.php (1)
12-12: LGTM!The TaskServiceProvider is correctly registered following the same pattern as other extension providers.
app/Providers/Extensions/TaskServiceProvider.php (1)
17-30: LGTM!The service provider correctly registers TaskService as a singleton and wires up all default task schemas with their dependencies resolved from the container. The implementation follows Laravel best practices.
app/Extensions/Tasks/Schemas/CreateBackupSchema.php (1)
9-32: LGTM!The implementation correctly integrates with
InitiateBackupService. The payload parsing and ignored files handling are sound, andcanCreate()properly enforces the backup limit constraint.app/Extensions/Tasks/Schemas/DeleteFilesSchema.php (1)
8-26: LGTM!The schema correctly delegates file deletion to
DeleteFilesService, with payload parsing that aligns with the expected newline-delimited file patterns.app/Extensions/Tasks/Schemas/TaskSchema.php (1)
10-52: LGTM!The abstract base class provides sensible defaults for task schemas. The
formatPayload()implementation usingempty()andexplode()is appropriate for the base case, with concrete implementations free to override as needed (e.g.,PowerActionSchemaoverrides to format differently).app/Extensions/Tasks/TaskService.php (1)
18-21: The original review comment is based on an incorrect assumption about the codebase.The
get()method cannot return null for any task that passes validation. All four valid task actions (power,command,backup,delete_files) have corresponding schemas registered inTaskServiceProviderduring application bootstrap. TheTaskmodel validates that theactionfield matches one of these four values, so$taskService->get($this->task->action)will always resolve to a valid schema. No null check is needed, and no bug exists here.Likely an incorrect or invalid review comment.
app/Extensions/Tasks/Schemas/SendCommandSchema.php (2)
26-35: Good override of base class form component.Using
TextInputwithrequired()validation is more appropriate for single-line commands than the base class'sTextarea. This provides better UX for command input.
16-19: No issues found—the Server::send() method is properly implemented.The verification confirms that
Server::send()exists at line 452 with the correct signaturepublic function send(array|string $command): ResponseInterface. The method properly handles the payload passed fromSendCommandSchema::runTask(), accepting either array or string commands and returning a ResponseInterface as expected.app/Extensions/Tasks/Schemas/PowerActionSchema.php (3)
13-13: Good use of dependency injection.Constructor properly injects the DaemonServerRepository, following Laravel best practices and enabling testability.
20-23: LGTM - Correct repository usage.The runTask implementation properly chains
setServer()andpower()methods on the DaemonServerRepository, which are confirmed to exist in the repository class.
47-51: Power action values are correct.The hardcoded power actions (start, restart, stop, kill) match the daemon API exactly. The Pelican Panel daemon API accepts start, stop, restart, and kill, and all four values are present in the schema.
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (1)
44-49: Excellent dynamic form implementation.The refactored form builder elegantly uses TaskSchemaInterface to:
- Dynamically populate action options via
Arr::mapWithKeys- Conditionally disable unavailable actions with
canCreate()- Inject schema-specific payload forms via
Group::makeclosure- Set appropriate defaults from schemas
This design significantly improves extensibility over hardcoded options.
app/Extensions/Tasks/TaskSchemaInterface.php (1)
9-28: Well-designed interface for extensible task system.The TaskSchemaInterface provides a clean contract that:
- Separates task identity, execution, and UI concerns
- Supports conditional task creation via
canCreate()- Allows flexible payload formatting (string|array|null)
- Enables dynamic form generation via
getPayloadForm()This design provides excellent extensibility for custom task types while maintaining type safety.
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (1)
71-72: Consider dependency injection for TaskService.While the
app()helper is suppressed and works correctly, injectingTaskServicevia constructor or method parameter would be more testable and align with Laravel best practices.Example using method injection:
public function table(Table $table, TaskService $taskService): Table { /** @var Schedule $schedule */ $schedule = $this->getOwnerRecord(); $tasks = $taskService->getAll(); // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (6)
app/Extensions/Tasks/TaskService.php (3)
TaskService(5-31)get(18-21)getAll(13-16)app/Models/Task.php (1)
Task(27-123)app/Extensions/Tasks/Schemas/CreateBackupSchema.php (3)
canCreate(23-26)getId(13-16)getPayloadLabel(28-31)app/Extensions/Tasks/TaskSchemaInterface.php (7)
canCreate(17-17)getId(11-11)getName(13-13)getDefaultPayload(19-19)getPayloadForm(27-27)formatPayload(24-24)getPayloadLabel(21-21)app/Extensions/Tasks/Schemas/TaskSchema.php (6)
canCreate(17-20)getName(12-15)getDefaultPayload(22-25)getPayloadForm(43-51)formatPayload(33-40)getPayloadLabel(27-30)app/Extensions/Tasks/Schemas/PowerActionSchema.php (5)
getId(15-18)getDefaultPayload(25-28)getPayloadForm(41-56)formatPayload(35-38)getPayloadLabel(30-33)
🔇 Additional comments (2)
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (2)
5-6: LGTM!The new imports are necessary for the dynamic task schema system and are correctly used throughout the file.
Also applies to: 18-19, 25-25
78-86: LGTM! Unregistered task actions are now handled gracefully.The null coalescing operators on lines 80, 83, and 84 properly handle cases where a task references an action not registered in TaskService, falling back to raw values when schemas are missing. This addresses the concern from the previous review.
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php
Outdated
Show resolved
Hide resolved
Boy132
force-pushed
the
boy132/refactor-tasks
branch
from
6766151 to
c3c53e3
Compare
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (2)
45-50: Add defensive checks for array access (previously reported).The direct array access issues flagged in the previous review are still present. Without null-safe operators:
- Line 45:
$tasks[$value]->canCreate()will throw if$valuenot in$tasks- Line 48:
array_key_first($tasks)returns null if empty- Line 49:
$tasks[$state]->getDefaultPayload()will throw if$statenot in$tasks- Line 50:
$tasks[$get('action')]->getPayloadForm()will throw if action not in$tasksWhile uncommon, this can occur if a task references an unregistered action or TaskService has no schemas.
Apply defensive checks as previously suggested:
->disableOptionWhen(fn (string $value) => !($tasks[$value] ?? null)?->canCreate($schedule)) ->options($taskService->getMappings()) ->selectablePlaceholder(false) -->default(array_key_first($tasks)) -->afterStateUpdated(fn ($state, Set $set) => $set('payload', $tasks[$state]->getDefaultPayload())), -Group::make(fn (Get $get) => $tasks[$get('action')]->getPayloadForm()), +->default(array_key_first($tasks) ?? '') +->afterStateUpdated(fn ($state, Set $set) => $set('payload', $tasks[$state]?->getDefaultPayload())), +Group::make(fn (Get $get) => $tasks[$get('action')]?->getPayloadForm() ?? []),
81-89: Handle unregistered task actions gracefully (previously reported).Direct array access on lines 83, 86, and 87 will throw errors if
$task->actionis not registered in TaskService. This can occur with legacy tasks, invalid database data, or removed schemas.Apply defensive checks as previously suggested:
TextColumn::make('action') ->label(trans('server/schedule.tasks.actions.title')) - ->state(fn (Task $task) => $tasks[$task->action]->getName()), + ->state(fn (Task $task) => $tasks[$task->action]?->getName() ?? $task->action), TextColumn::make('payload') ->label(trans('server/schedule.tasks.payload')) - ->state(fn (Task $task) => $tasks[$task->action]->formatPayload($task->payload)) - ->tooltip(fn (Task $task) => $tasks[$task->action]->getPayloadLabel()) + ->state(fn (Task $task) => $tasks[$task->action]?->formatPayload($task->payload) ?? $task->payload) + ->tooltip(fn (Task $task) => $tasks[$task->action]?->getPayloadLabel()) ->placeholder(trans('server/schedule.tasks.no_payload')) ->badge(),
🧹 Nitpick comments (1)
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (1)
72-75: Consider caching TaskService instance.The table method fetches tasks independently from
getTaskForm. While not critical, you might consider reusing the same instance to improve consistency.For example:
public function table(Table $table): Table { $schedule = $this->getOwnerRecord(); $taskService = app(TaskService::class); // @phpstan-ignore myCustomRules.forbiddenGlobalFunctions $tasks = $taskService->getAll();Could become a class property or helper method to reduce duplication, though the current approach is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Extensions/Tasks/TaskService.php(1 hunks)app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Extensions/Tasks/TaskService.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-07T14:44:18.583Z
Learnt from: notAreYouScared
Repo: pelican-dev/panel PR: 1779
File: app/Filament/Admin/Resources/Users/Pages/EditUser.php:51-51
Timestamp: 2025-10-07T14:44:18.583Z
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/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php
🧬 Code graph analysis (1)
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (9)
app/Extensions/Tasks/TaskService.php (4)
TaskService(5-37)getAll(13-16)getMappings(33-36)get(18-21)app/Models/Schedule.php (2)
Schedule(38-145)tasks(133-136)app/Models/Task.php (1)
Task(27-123)app/Extensions/Tasks/Schemas/CreateBackupSchema.php (2)
canCreate(23-26)getPayloadLabel(28-31)app/Extensions/Tasks/TaskSchemaInterface.php (6)
canCreate(17-17)getDefaultPayload(19-19)getPayloadForm(27-27)getName(13-13)formatPayload(24-24)getPayloadLabel(21-21)app/Extensions/Tasks/Schemas/TaskSchema.php (6)
canCreate(17-20)getDefaultPayload(22-25)getPayloadForm(43-51)getName(12-15)formatPayload(33-40)getPayloadLabel(27-30)app/Extensions/Tasks/Schemas/PowerActionSchema.php (4)
getDefaultPayload(25-28)getPayloadForm(41-56)formatPayload(35-38)getPayloadLabel(30-33)app/Extensions/Tasks/Schemas/SendCommandSchema.php (2)
getPayloadForm(27-35)getPayloadLabel(21-24)app/Extensions/Tasks/Schemas/DeleteFilesSchema.php (1)
getPayloadLabel(22-25)
🔇 Additional comments (3)
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (3)
5-6: LGTM! Imports support the new task schema system.The new imports for TaskService, TaskSchemaInterface, and Filament schema components are appropriate for the dynamic task registration pattern.
Also applies to: 18-19
102-102: LGTM! TaskService correctly propagated to form builder.Both create and edit actions properly pass the TaskService instance to
getTaskForm, maintaining consistent dependency injection throughout the relation manager.Also applies to: 135-135
36-38: The review comment is based on an incorrect assumption about TaskService initialization.The concern about empty
$tasksbreakingarray_key_first()is not valid becauseTaskServiceProviderregisters four default task schemas (PowerActionSchema,SendCommandSchema,CreateBackupSchema,DeleteFilesSchema) in the singleton during bootstrap. ThegetTaskForm()method receives the initializedTaskServicevia dependency injection, guaranteeing at least four schemas are always available. For$tasksto be empty would require a fundamental bootstrap failure rather than a logic error in this method.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (2)
43-48: Array access safety concerns remain unaddressed.The direct array access to
$tasks[$value],$tasks[$state], and$tasks[$get('action')]will cause runtime errors if the task registry is empty or if keys are missing. While Filament's Select component constrains valid selections, edge cases (empty registry, dynamic schema removal, or database inconsistencies) can still trigger these errors.
81-85: Array access safety concerns remain unaddressed.Direct array access to
$tasks[$task->action]at lines 81, 84, and 85 will cause runtime errors if a Task references an action not registered in TaskService. This can occur if tasks exist in the database for removed or unregistered schemas. Although a previous review comment was marked as addressed, the defensive checks are not present in the current code.
🧹 Nitpick comments (1)
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (1)
70-73: Consider encapsulating task retrieval logic in TaskService.Following the pattern suggested by rmartinossar and similar to AvatarService, consider adding helper methods to TaskService that encapsulate common operations like retrieving task names, formatting payloads, and getting payload labels. This would centralize defensive checks and reduce code duplication across consumers.
For example, TaskService could provide:
public function getTaskName(string $action): string { return $this->get($action)?->getName() ?? $action; } public function formatPayload(string $action, string $payload): null|string|array { return $this->get($action)?->formatPayload($payload) ?? $payload; }This would simplify the relation manager and make defensive programming the default.
Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-07T14:44:18.583Z
Learnt from: notAreYouScared
Repo: pelican-dev/panel PR: 1779
File: app/Filament/Admin/Resources/Users/Pages/EditUser.php:51-51
Timestamp: 2025-10-07T14:44:18.583Z
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/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php
🧬 Code graph analysis (1)
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (8)
app/Extensions/Tasks/TaskService.php (4)
TaskService(5-37)getAll(13-16)getMappings(33-36)get(18-21)app/Models/Task.php (1)
Task(27-123)app/Extensions/Tasks/Schemas/CreateBackupSchema.php (2)
canCreate(23-26)getPayloadLabel(28-31)app/Extensions/Tasks/TaskSchemaInterface.php (6)
canCreate(17-17)getDefaultPayload(19-19)getPayloadForm(27-27)getName(13-13)formatPayload(24-24)getPayloadLabel(21-21)app/Extensions/Tasks/Schemas/TaskSchema.php (6)
canCreate(17-20)getDefaultPayload(22-25)getPayloadForm(43-51)getName(12-15)formatPayload(33-40)getPayloadLabel(27-30)app/Extensions/Tasks/Schemas/PowerActionSchema.php (4)
getDefaultPayload(25-28)getPayloadForm(41-56)formatPayload(35-38)getPayloadLabel(30-33)app/Extensions/Tasks/Schemas/SendCommandSchema.php (2)
getPayloadForm(27-35)getPayloadLabel(21-24)app/Extensions/Tasks/Schemas/DeleteFilesSchema.php (1)
getPayloadLabel(22-25)
When deleting last task then you can't create new ones after.
|
| ->default('restart'), | ||
| ->default(array_key_first($tasks)) | ||
| ->afterStateUpdated(fn ($state, Set $set) => $set('payload', $tasks[$state]->getDefaultPayload())), | ||
| Group::make(fn (Get $get) => array_key_exists($get('action'), $tasks) ? $tasks[$get('action')]->getPayloadForm() : []), |
There was a problem hiding this comment.
Is it even worth adding the group if the task schema doesn't provide payloadForm ?
There was a problem hiding this comment.
Yes, there always is a payload form. The array_key_exists is only necessary because of a little race condition: the group schema is loaded before the default value, so for a short time $get('action') will return an empty string instead of the default task type.
# Conflicts: # app/Models/Task.php
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/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (1)
132-142: I'll verify the PHP/Laravel behavior related to the null dereference claim and the proposed fix.Let me search for definitive information on Laravel's aggregate max() behavior with empty results.
Fix sequence ID calculation when a schedule has no tasks (null dereference).
The Laravel query builder's first() method will always return one result or null if no results are found. The original code attempts to access the
sequence_idproperty on this potentially null value before the null coalescing operator (??) is evaluated. Due to PHP operator precedence, property access (the->operator) has higher precedence than the null coalescing operator (??), which means accessing->sequence_idon null will throw an error before?? 0can protect it.This is a critical bug matching the reported issue. The proposed fix correctly uses
max('sequence_id')instead, which in accordance with SQL standards returns NULL when there are no values to aggregate. This makes the null coalescing operator effective:
- When tasks exist:
max()returns the integer value → works correctly- When no tasks:
max()returns NULL →NULL ?? 0safely defaults to 0Apply the fix:
-$sequenceId = ($schedule->tasks()->orderByDesc('sequence_id')->first()->sequence_id ?? 0) + 1; +$sequenceId = ($schedule->tasks()->max('sequence_id') ?? 0) + 1;
♻️ Duplicate comments (1)
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (1)
30-52: Dynamic task form wiring via TaskService looks solid; DI and extra guards are optional.Using
TaskService::getAll()/getMappings()to drive theSelectandGroupis consistent with the new schema system, and thearray_key_exists($get('action'), $tasks)guard nicely avoids undefined indexes during the brief empty‑state race. BecausedisableOptionWhenandafterStateUpdatedonly see keys coming from the same$tasksarray used foroptions(), the direct$tasks[$value]/$tasks[$state]access is safe in the current design. If you later support runtime schema removal or more dynamic registries, you could switch those to null‑safe access with a fallback default and injectTaskServiceinstead of callingapp()here, but that’s purely an optional hardening / DI improvement and repeats the earlier DI suggestion for this service.
🧹 Nitpick comments (1)
app/Models/Task.php (1)
6-7: Task schema resolution via TaskService is correct; DI would just be a nicety.
getSchema()cleanly delegates toTaskService::get()and the nullable return type matches the null‑safe usage in the table, so unknown actions won’t break rendering. If you want to tighten design further, you could injectTaskService(or cache it in a static property) instead of callingapp()on every call, but that’s an optional refinement rather than a blocker.Also applies to: 120-126
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php(3 hunks)app/Models/Task.php(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-07T14:44:18.583Z
Learnt from: notAreYouScared
Repo: pelican-dev/panel PR: 1779
File: app/Filament/Admin/Resources/Users/Pages/EditUser.php:51-51
Timestamp: 2025-10-07T14:44:18.583Z
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/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php
🧬 Code graph analysis (2)
app/Models/Task.php (1)
app/Extensions/Tasks/TaskService.php (1)
TaskService(5-37)
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (6)
app/Extensions/Tasks/TaskService.php (4)
TaskService(5-37)getAll(13-16)getMappings(33-36)get(18-21)app/Models/Task.php (3)
Task(29-127)schedule(95-98)getSchema(120-126)app/Extensions/Tasks/Schemas/CreateBackupSchema.php (2)
canCreate(23-26)getPayloadLabel(28-31)app/Extensions/Tasks/Schemas/PowerActionSchema.php (4)
getDefaultPayload(25-28)getPayloadForm(41-56)formatPayload(35-38)getPayloadLabel(30-33)app/Extensions/Tasks/Schemas/SendCommandSchema.php (2)
getPayloadForm(27-35)getPayloadLabel(21-24)app/Extensions/Tasks/Schemas/DeleteFilesSchema.php (1)
getPayloadLabel(22-25)
🔇 Additional comments (1)
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (1)
77-84: Table columns now use Task::getSchema() safely with graceful fallbacks.Resolving the schema via
$task->getSchema()?->…and falling back to$task->action/$task->payloadensures that unknown or unregistered actions no longer cause array‑index errors while still rendering something meaningful in the UI.
Same principle as oauth, captcha and avatars. Allows to register custom schedule tasks.