Improve "first task" checks

[Boy132]

Closes #1922

[coderabbitai]

📝 Walkthrough

Walkthrough

Replaces sequence_id-based first-task checks with explicit helpers: adds Schedule::firstTask() and Task::isFirst(), updates TasksRelationManager to use isFirst() for form/table time_offset logic, and switches ProcessScheduleService to call firstTask() instead of a direct query.

Changes

Cohort / File(s)	Summary
Models — first-task helpers app/Models/Schedule.php, app/Models/Task.php	Added Schedule::firstTask(): ?Task to return the first task ordered by sequence_id, and Task::isFirst(): bool to check if a task is the schedule's first task.
Filament Relation Manager — time_offset logic app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php	Updated TextInput::make('time_offset') hidden callback to accept an optional Task and hide based on isFirst(); changed TextColumn::make('time_offset') state callback to use isFirst() and adjusted suffix to accept the computed state.
Service — schedule processing app/Services/Schedules/ProcessScheduleService.php	Replaced direct query $schedule->tasks()->orderBy('sequence_id')->first() with $schedule->firstTask() while keeping downstream logic unchanged.

Sequence Diagram(s)

sequenceDiagram
    participant UI as Filament UI
    participant TM as TasksRelationManager
    participant Task as Task model
    participant Schedule as Schedule model
    participant Service as ProcessScheduleService

    rect rgb(220,240,255)
    note right of UI: Rendering form/table or processing schedule
    UI->>TM: Render time_offset field / column
    TM->>Task: Call isFirst()
    Task->>Schedule: Call firstTask()
    Schedule-->>Task: Return first task (by sequence_id)
    Task-->>TM: isFirst() result
    TM-->>UI: Hide/format time_offset based on isFirst()
    end

    rect rgb(240,255,230)
    note right of Service: When processing schedule
    Service->>Schedule: Call firstTask()
    Schedule-->>Service: Return first task
    Service->>Service: Continue processing with returned task
    end

Loading

Possibly related PRs

• Improve time offset ux #1772: Modifies TasksRelationManager time_offset handling (form hiding and table display); directly related to the same UI logic changes.

Pre-merge checks

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The pull request adds isFirst() methods and refactors first-task checks to use them, but does not address the core issue of sequence_id not being updated when tasks are deleted.	Add logic to update sequence_id values when tasks are deleted, ensuring the first task never retains an offset; consider implementing this in the delete handler of TasksRelationManager.
Docstring Coverage	⚠️ Warning	Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The pull request title 'Improve "first task" checks' accurately describes the main change, which refactors first-task detection to use a dedicated isFirst() method instead of sequence_id comparisons.
Description check	✅ Passed	The pull request description references issue #1922, which relates to the changeset; it is minimal but sufficient to indicate the linked issue being addressed.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to refactoring first-task checks via the new isFirst() method; no out-of-scope modifications were detected.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

app/Models/Task.php (1)

124-127: Clean implementation of first-task check.

The logic correctly determines if the task is first in its schedule by comparing IDs. The null-safe operator properly handles the edge case when firstTask() returns null.

Optional performance optimization:

If isFirst() is called frequently or on multiple tasks in a loop, consider eager-loading the schedule relationship to avoid N+1 queries. However, this is not critical for the current implementation.

app/Models/Schedule.php (1)

146-152: Method works correctly but could leverage eager-loaded tasks.

The implementation correctly returns the first task by sequence_id.

Optional optimization:

Since the Schedule model eager-loads tasks (line 52: protected $with = ['tasks']), consider using the loaded collection to avoid an extra query:

 public function firstTask(): ?Task
 {
-    /** @var ?Task $task */
-    $task = $this->tasks()->orderBy('sequence_id')->first();
-
-    return $task;
+    return $this->tasks->sortBy('sequence_id')->first();
 }

This eliminates a database query when tasks are already loaded. Note that sortBy on a collection returns a new collection, so the original remains unaffected.

app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (1)

75-75: Redundant first-task checks in hidden condition.

The condition checks both $get('sequence_id') === 1 and $task?->isFirst(), which appear redundant given the PR's goal to replace sequence-based checks with isFirst().

Consider simplifying to rely solely on isFirst():

-->hidden(fn (Get $get, ?Task $task) => config('queue.default') === 'sync' || $get('sequence_id') === 1 || $schedule->tasks->isEmpty() || $task?->isFirst())
+->hidden(fn (Get $get, ?Task $task) => config('queue.default') === 'sync' || $schedule->tasks->isEmpty() || $task?->isFirst())

This aligns with the PR objective to use dynamic first-task detection instead of static sequence_id comparisons.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 25c8ff3 and e845b0e.

📒 Files selected for processing (4)

• app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (2 hunks)
• app/Models/Schedule.php (1 hunks)
• app/Models/Task.php (1 hunks)
• app/Services/Schedules/ProcessScheduleService.php (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (4)

app/Services/Schedules/ProcessScheduleService.php (2)

app/Models/Task.php (1)

• schedule (104-107)

app/Models/Schedule.php (1)

• firstTask (146-152)

app/Models/Task.php (1)

app/Models/Schedule.php (1)

• firstTask (146-152)

app/Models/Schedule.php (1)

app/Models/Task.php (1)

• Task (27-128)

app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (2)

app/Models/Task.php (3)

• Task (27-128)
• schedule (104-107)
• isFirst (124-127)

app/Models/Schedule.php (1)

• tasks (133-136)

🔇 Additional comments (2)

app/Services/Schedules/ProcessScheduleService.php (1)

24-24: LGTM! Clean refactoring to use the new abstraction.

The change to use $schedule->firstTask() encapsulates the first-task retrieval logic and maintains identical behavior.

app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (1)

111-112: I'll verify this by examining the actual file and searching for documentation about Filament's TextColumn behavior with null states.

Now let me search for information about Laravel's trans_choice function and how it handles null values:

Let me search more specifically for Filament's suffix behavior with null states:

Now let me examine the actual code file to understand the full context:Perfect! I found the key source code reference. Now I have enough information to provide a comprehensive final response.

Based on my research, I can now provide an informed rewritten review comment.

Suffix callback behavior with null state is not explicitly safeguarded in Filament.

The concern raised is partially valid but based on incomplete information. Filament's date formatting checks if state is blank and returns null, suggesting callbacks can receive null values. While formatStateUsing() does not execute when state is null, the suffix callback behavior with null states is not explicitly documented for TextColumn.

However, passing null values to trans() causes deprecation warnings in Laravel 8.1+, so if the suffix callback is invoked with null, trans_choice(null, $state) would indeed cause issues. Given that this appears to be a calculated/conditional column, a defensive approach is recommended:

-->suffix(fn ($state) => $state !== null ? ' ' . trans_choice('server/schedule.tasks.seconds', $state) : '')

Alternatively, ensure the state is never null by returning 0 for first tasks (e.g., $task->isFirst() ? 0 : $task->time_offset), which may be semantically appropriate.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (1)

75-75: First-task detection correctly implemented.

The change to use $task?->isFirst() correctly addresses the issue where the first task should not display a time offset field. The null-safe operator appropriately handles both creation and edit scenarios.

Note: This matches the suggestion from a previous review comment, confirming the approach is sound.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e845b0e and ef7ac34.

📒 Files selected for processing (1)

• app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (2)

app/Models/Task.php (3)

• Task (27-128)
• schedule (104-107)
• isFirst (124-127)

app/Models/Schedule.php (1)

• tasks (133-136)