Make sure default variable value is set and that variables are created when viewing server #1758
Conversation
…d when viewing server
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds Server::ensureVariablesExist() to seed missing ServerVariable records from egg variables. The Startup and EditServer Filament pages now invoke this method (via a relationship callback or repeater config) before returning the viewable, ordered server-variables query. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Startup Page (Filament)
participant S as Server
participant M as Server::ensureVariablesExist()
participant SV as ServerVariable Model
participant DB as Database
User->>UI: Open Startup page
UI->>S: get current Server
UI->>M: call ensureVariablesExist()
M->>SV: for each EggVariable -> firstOrCreate({server_id, variable_id}, {...})
SV->>DB: INSERT/SELECT (upsert)
DB-->>SV: persisted records
UI->>SV: query user_viewable ServerVariables ordered by variable.sort
SV->>DB: SELECT with joins/filters
DB-->>UI: Result set
UI-->>User: Render repeater with variables
sequenceDiagram
autonumber
actor Admin
participant UI as EditServer Page (Filament)
participant S as Server
participant M as Server::ensureVariablesExist()
participant SV as ServerVariable Model
participant DB as Database
Admin->>UI: Open/Edit server variables form
UI->>S: call ensureVariablesExist()
M->>SV: create missing ServerVariable records via firstOrCreate
SV->>DB: INSERT/SELECT (upsert)
DB-->>SV: persisted records
UI->>SV: query variables ordered by variable.sort
DB-->>UI: Result set
UI-->>Admin: Render form with seeded variables
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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
🧹 Nitpick comments (3)
app/Filament/Server/Pages/Startup.php (2)
105-113: Guard against null defaults to keepvariable_valuestringy.If both
server_valueanddefault_valueare null, you’ll persist null. GivenServerVariable::$validationRules['variable_value' => ['string']], prefer a safe empty string.Apply this diff:
- 'variable_value' => $variable->server_value ?? $variable->default_value, + 'variable_value' => $variable->server_value ?? ($variable->default_value ?? ''),
105-113: Avoid seeding inside a relationship callback (side‑effects on read + N+1).This runs a select/insert per variable on every render. Prefer:
- Seed when an EggVariable is created/attached (model observer/job), or
- A dedicated service method called on state changes, or
- A bulk upsert to minimize queries.
Example extraction (conceptual):
- foreach ($server->variables as $variable) { - ServerVariable::firstOrCreate([...]); - } + app(\App\Services\Servers\ServerVariableSeeder::class)->seedMissing($server);And in the service, compute the missing
(server_id, variable_id)pairs and perform a singleupsertorinsert ignorerather than per‑rowfirstOrCreate.app/Filament/Admin/Resources/Servers/Pages/EditServer.php (1)
640-649: Deduplicate seeding logic shared with Startup page.The foreach+firstOrCreate block now exists in two places (Startup and EditServer). Extract to a shared service/observer to avoid drift and reduce query count with a bulk strategy.
I can draft a small
ServerVariableSeederwith a bulk upsert if you’d like.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php(1 hunks)app/Filament/Server/Pages/Startup.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php (1)
app/Models/ServerVariable.php (2)
ServerVariable(21-65)variable(61-64)
app/Filament/Server/Pages/Startup.php (2)
app/Models/Server.php (2)
Server(132-504)variables(310-321)app/Models/ServerVariable.php (2)
variable(61-64)ServerVariable(21-65)
🔇 Additional comments (2)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php (1)
641-646: LGTM: directfirstOrCreate+ prefer variable default.
- Static
ServerVariable::firstOrCreateis fine vs::query()->firstOrCreate.- Using
$variable->server_value ?? $variable->default_valuealigns with the PR objective.If
default_valuecan be null in your schema, consider coalescing to empty string to keepvariable_valuenon‑null:- 'variable_value' => $variable->server_value ?? $variable->default_value, + 'variable_value' => $variable->server_value ?? ($variable->default_value ?? ''),app/Filament/Server/Pages/Startup.php (1)
105-116: Ensure relationship callback signature matches Filament version
YourStartup.phpusesfunction (Builder $query, Server $server), whereas other relationship callbacks (includingEditServer.php) use a single‐parameter closure and call$this->getRecord()inside. Confirm that your installed Filament version supports passing the record as the second argument; if not, revert to a one‐parameter closure and retrieve the record with$this->getRecord()to avoid runtime errors.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php (1)
633-642: Avoid writes inside relationship callbacks; seed earlier to reduce surprises and queries.Calling
$server->ensureServerVariablesExist()during the relationship query executes DB writes while building the form. Prefer seeding earlier (e.g., inmount(),beforeFill(), or at the start ofform()), so rendering is read-only and runs fewer queries per request. Keep ordering withorderByPowerJoins('variable.sort')unchanged.If you keep it here, at minimum wrap seeding to do nothing when no rows are missing to avoid repeated
firstOrCreateselects.Would you like a small patch moving the seeding to
mount()and keeping the same behavior?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php(1 hunks)app/Filament/Server/Pages/Startup.php(1 hunks)app/Models/Server.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Filament/Server/Pages/Startup.php
🧰 Additional context used
🧬 Code graph analysis (2)
app/Models/Server.php (1)
app/Models/ServerVariable.php (2)
variable(61-64)ServerVariable(21-65)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php (1)
app/Models/Server.php (1)
ensureServerVariablesExist(328-338)
Closes #1754