Create custom startup variable field #1615
Conversation
📝 WalkthroughWalkthroughAdds a StartupVariable Filament form field (class, Blade view, enum) and replaces inline TextInput/Select logic in ServerResource CreateServer/EditServer with this field, centralizing type inference, select option parsing, and enabling live validation on blur. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin User
participant Form as Filament Form (Server Create/Edit)
participant Field as StartupVariable Field
participant Validator as Live Validation
participant Livewire as Livewire State
rect rgb(230,245,255)
Admin->>Form: Open server variables step / interact with repeater
Form->>Field: Render with variable metadata & rules
Field->>Field: Infer type (text/select/toggle) and options from rules
end
rect rgb(235,255,230)
Admin->>Field: Enter value and blur
Field->>Validator: Trigger live validation (on blur)
Validator-->>Field: Return errors or success
Field->>Livewire: Update state and surface inline errors
end
rect rgb(255,243,230)
Admin->>Form: Attempt Next Step / Submit
Form->>Validator: Run step/submit validation (includes field rules)
Validator-->>Form: Return errors or proceed
end
Assessment against linked issues
Out-of-scope changes
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
app/Filament/Admin/Resources/ServerResource/Pages/CreateServer.php (1)
328-337: Synchronizeenvironmentwith edits toserver_variables
Theenvironmentarray is only initialized when the egg selection changes, so any user edits to individual variable values in the repeater aren’t reflected at submission time. SinceServerCreationService::handle()usesArr::get($data, 'environment', [])to validate and store variables, stale values may be persisted.Please update the repeater definition in:
app/Filament/Admin/Resources/ServerResource/Pages/CreateServer.phparound theRepeater::make('server_variables')block (lines ~328–337)- And likewise in the edit page (lines 429–443)
Add the following to the repeater schema:
->hidden(fn ($state) => empty($state)) + ->live() + ->afterStateUpdated(function ($state, Set $set) { + $env = collect($state ?? []) + ->filter(fn ($item) => isset($item['env_variable'])) + ->mapWithKeys(fn ($item) => [ + $item['env_variable'] => $item['variable_value'] ?? '', + ]) + ->all(); + $set('environment', $env); + }) ->schema([ StartupVariable::make('variable_value'), ])This ensures that any adjustments to
variable_valueimmediately update theenvironmentpayload.
🧹 Nitpick comments (4)
app/Enums/StartupVariableType.php (1)
5-10: Enum looks good; ensure Toggle UI is implemented or gatedThe Toggle case is defined but currently not rendered in the Blade view. Either implement Toggle rendering in the Blade (preferred) or temporarily gate
getType()from returningToggleuntil the Blade supports it to avoid a confusing UX for boolean variables.resources/views/filament/components/startup-variable.blade.php (2)
19-21: Add class handling for Toggle (or reuse select styling for now)Right now classes only handle Text and Select. If you render Toggle as a select fallback, also treat Toggle as
fi-fo-selectfor consistent styling; otherwise add a dedicated class for Toggle.Apply this minimal change to include Toggle under the select styling:
- 'fi-fo-select' => $type === \App\Enums\StartupVariableType::Select + 'fi-fo-select' => $type === \App\Enums\StartupVariableType::Select || $type === \App\Enums\StartupVariableType::Toggle
41-55: Accessibility: add aria-invalid on text input for clearer error signallingYou already set the wrapper’s valid state. Propagate
aria-invaliddown to the input for assistive tech.For example:
<x-filament::input :attributes=" \Filament\Support\prepare_inherited_attributes($getExtraInputAttributeBag()) ->merge($getExtraAlpineAttributes(), escape: false) ->merge([ 'id' => $getId(), 'required' => $isRequired(), 'disabled' => $isDisabled, 'placeholder' => $getVariableDefault(), $applyStateBindingModifiers('wire:model') => $statePath, + 'aria-invalid' => $errors->has($statePath), ], escape: false) " />app/Filament/Components/Forms/Fields/StartupVariable.php (1)
140-145: Strict in_array for required rule (minor)Use strict comparison to avoid edge cases with non-string rule values.
- return in_array('required', $rules); + return in_array('required', $rules, true);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
app/Enums/StartupVariableType.php(1 hunks)app/Filament/Admin/Resources/ServerResource/Pages/CreateServer.php(3 hunks)app/Filament/Admin/Resources/ServerResource/Pages/EditServer.php(3 hunks)app/Filament/Components/Forms/Fields/StartupVariable.php(1 hunks)resources/views/filament/components/startup-variable.blade.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
app/Filament/Components/Forms/Fields/StartupVariable.php (2)
app/Models/ServerVariable.php (2)
ServerVariable(20-64)variable(60-63)app/Filament/Server/Pages/ServerFormPage.php (1)
getRecord(66-72)
app/Filament/Admin/Resources/ServerResource/Pages/CreateServer.php (1)
app/Filament/Components/Forms/Fields/StartupVariable.php (1)
StartupVariable(14-178)
app/Filament/Admin/Resources/ServerResource/Pages/EditServer.php (1)
app/Filament/Components/Forms/Fields/StartupVariable.php (1)
StartupVariable(14-178)
🔇 Additional comments (4)
app/Filament/Admin/Resources/ServerResource/Pages/CreateServer.php (1)
430-443: Good refactor: centralized per-variable field with early validation behaviorReplacing the inline schema with
StartupVariable::make('variable_value')simplifies the form and restores field-level validation via the component (->rules()and->live(onBlur: true)). This aligns with the objective to validate on “Next Step”.app/Filament/Admin/Resources/ServerResource/Pages/EditServer.php (2)
10-10: Import of StartupVariable is appropriateThe import is correct and matches the new field usage below.
617-644: Cleaner relationship save and field usage
- Using
mutateRelationshipDataBeforeSaveUsingto guaranteevariable_valueexists is concise and safe.- Replacing the dynamic inline schema with
StartupVariablesimplifies maintenance and keeps behavior consistent with CreateServer.app/Filament/Components/Forms/Fields/StartupVariable.php (1)
39-52: Great: centralized rules and on-blur live validationSetting rules and enabling
live(onBlur: true)on the field directly is exactly what’s needed to restore early validation on step transition and blur.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
app/Filament/Components/Forms/Fields/StartupVariable.php (2)
148-161: Resolved: using Arr::first over array_first — LGTMThanks for switching to Illuminate\Support\Arr::first; this avoids reliance on helpers and aligns with framework APIs.
163-178: Bug: getSelectOptions doesn’t trim values (each() doesn’t transform) and Str::trim() is invalid
- each() does not map/transform collection items; options remain untrimmed, leading to select/validation mismatches.
- Str::trim() is not a valid static method; use PHP’s trim() or Stringable.
Apply this diff:
$inRule = Arr::first($rules, fn ($value) => str($value)->startsWith('in:')); if ($inRule) { return str($inRule) ->after('in:') ->explode(',') - ->each(fn ($value) => Str::trim($value)) + ->map(fn ($value) => trim($value)) ->all(); }
🧹 Nitpick comments (3)
app/Filament/Components/Forms/Fields/StartupVariable.php (3)
39-52: Polish UX: null-safe prefix, required asterisk, and default value wiring in setUp()Tiny improvements to reduce noise and improve clarity:
- Avoid showing "{{}}" when env is missing.
- Show a required asterisk for required variables.
- Pre-fill with the variable’s default when no state is set.
- Don’t render a placeholder em-dash for empty helper text.
Apply this diff:
$this->label(fn (StartupVariable $component) => $component->getVariableName()); - $this->prefix(fn (StartupVariable $component) => '{{' . $component->getVariableEnv() . '}}'); + $this->prefix(fn (StartupVariable $component) => ($env = $component->getVariableEnv()) ? '{{' . $env . '}}' : null); $this->hintIcon('tabler-code'); $this->hintIconTooltip(fn (StartupVariable $component) => implode('|', $component->getVariableRules())); - $this->helperText(fn (StartupVariable $component) => !$component->getVariableDesc() ? '—' : $component->getVariableDesc()); + $this->helperText(fn (StartupVariable $component) => $component->getVariableDesc() ?: null); $this->rules(fn (StartupVariable $component) => $component->getVariableRules()); + $this->required(fn (StartupVariable $component) => $component->isRequired()); + $this->default(fn (StartupVariable $component) => $component->getVariableDefault()); $this->live(onBlur: true);
90-129: Guard against missing relation to avoid noticesIf the ServerVariable->variable relation isn’t loaded or is null, these accessors can raise notices in edge cases. Null-safe access keeps things resilient.
Apply this diff:
- if ($record instanceof ServerVariable) { - return $record->variable->name; - } + if ($record instanceof ServerVariable) { + return $record->variable?->name; + } @@ - if ($record instanceof ServerVariable) { - return $record->variable->description; - } + if ($record instanceof ServerVariable) { + return $record->variable?->description; + } @@ - if ($record instanceof ServerVariable) { - return $record->variable->env_variable; - } + if ($record instanceof ServerVariable) { + return $record->variable?->env_variable; + } @@ - if ($record instanceof ServerVariable) { - return $record->variable->default_value; - } + if ($record instanceof ServerVariable) { + return $record->variable?->default_value; + }
131-139: Defensive default for rules when relation is missingEnsure rules always resolve to an array even if the relation is absent.
Apply this diff:
- if ($record instanceof ServerVariable) { - return $record->variable->rules; - } + if ($record instanceof ServerVariable) { + return (array) ($record->variable?->rules ?? []); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/Filament/Components/Forms/Fields/StartupVariable.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/Filament/Components/Forms/Fields/StartupVariable.php (2)
app/Models/ServerVariable.php (2)
ServerVariable(20-64)variable(60-63)app/Filament/Server/Pages/ServerFormPage.php (1)
getRecord(66-72)
🔇 Additional comments (2)
app/Filament/Components/Forms/Fields/StartupVariable.php (2)
51-51: Early on-blur validation meets #1602 — LGTMUsing live(onBlur: true) restores immediate validation feedback during the server creation flow. This aligns with the early validation objective.
156-158: Confirm Toggle rendering is supported in the Blade viewgetType() can return Toggle, but ensure the Blade view renders a toggle input for this case; otherwise booleans will degrade to Text/Select.
If the toggle branch isn’t implemented yet in resources/views/filament/components/startup-variable.blade.php, I can propose the Blade changes to add it.
| { | ||
| case Text = 'text'; | ||
| case Select = 'select'; | ||
| case Toggle = 'toggle'; // TODO: add toggle to blade view |
There was a problem hiding this comment.
Should we wait for v4 since they made it a blade component ?
There was a problem hiding this comment.
Creating servers is also broken in v4, this probably fixes it in both
There was a problem hiding this comment.
Creating servers is also broken in v4, this probably fixes it in both
Yes.
|
I'd like the possibility of having a $this->selectablePlaceholder(fn (StartupVariable $component) => !$component->isRequired()) |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/Filament/Components/Forms/Fields/StartupVariable.php (1)
165-180: Trim options correctly; each() doesn’t transform values.You previously received the same suggestion; it still appears here. Using each() won’t trim option strings. Switch to map() (or array_map) to avoid mismatches and display issues.
Apply this diff:
$inRule = Arr::first($rules, fn ($value) => str($value)->startsWith('in:')); if ($inRule) { return str($inRule) ->after('in:') ->explode(',') - ->each(fn ($value) => Str::trim($value)) + ->map(fn ($value) => trim($value)) ->all(); }If this was already fixed in another branch/commit, please rebase this file so the change lands here as well.
🧹 Nitpick comments (5)
app/Filament/Admin/Resources/ServerResource/Pages/EditServer.php (3)
618-618: Hidden label: consider screen-reader accessibility.Hiding the label improves visual density but can hurt accessibility. Ensure the inner field wrapper or Blade view provides an aria-label or sr-only label so assistive tech still announces the variable name.
If you want, I can scan the Blade view to confirm an accessible label is rendered for each repeater item.
635-639: Normalize/trim values before save without masking booleans.The null-coalescing assignment avoids nulls in the DB, which is good. We can harden this by trimming strings while preserving booleans and other scalar types.
Apply this diff:
- ->mutateRelationshipDataBeforeSaveUsing(function (array $data): array { - $data['variable_value'] ??= ''; + ->mutateRelationshipDataBeforeSaveUsing(function (array $data): array { + $value = $data['variable_value'] ?? ''; + // Trim strings, leave non-strings as-is (e.g., boolean from Toggle type) + $data['variable_value'] = is_string($value) ? trim($value) : $value;
641-644: Expose a selectable placeholder when the variable is not required (prevents auto-selecting the first option).Review feedback asked to avoid defaulting to the first option when the field is optional. Once the field supports a selectable placeholder flag, wire it here.
Apply this diff (assuming the field gains a selectablePlaceholder() API — see my comment in StartupVariable.php):
- StartupVariable::make('variable_value') - ->fromRecord(), + StartupVariable::make('variable_value') + ->fromRecord() + ->selectablePlaceholder(fn (StartupVariable $component) => !$component->isRequired()),I can also patch the Blade view to render a null/disabled placeholder option when this flag is true — say the word and I’ll include the snippet.
app/Filament/Components/Forms/Fields/StartupVariable.php (2)
143-148: Make required detection robust to rule variants.Some eggs use conditional required rules (required_if, required_without, etc.). If you want the UI to treat those as “required-ish,” match any rule starting with “required”.
Apply this diff:
- return in_array('required', $rules); + return collect($rules)->contains(fn (string $r) => str($r)->startsWith('required'));If strict “required only” is intentional for UX reasons, feel free to skip this.
38-49: Tooltip readability nit: add spaces between rules.Minor readability tweak: use " | " between rules instead of "|".
Apply this diff:
- $this->hintIconTooltip(fn (StartupVariable $component) => implode('|', $component->getVariableRules())); + $this->hintIconTooltip(fn (StartupVariable $component) => implode(' | ', $component->getVariableRules()));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
app/Filament/Admin/Resources/ServerResource/Pages/CreateServer.php(3 hunks)app/Filament/Admin/Resources/ServerResource/Pages/EditServer.php(3 hunks)app/Filament/Components/Forms/Fields/StartupVariable.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Filament/Admin/Resources/ServerResource/Pages/CreateServer.php
🧰 Additional context used
🧬 Code graph analysis (2)
app/Filament/Components/Forms/Fields/StartupVariable.php (1)
app/Models/ServerVariable.php (2)
ServerVariable(20-64)variable(60-63)
app/Filament/Admin/Resources/ServerResource/Pages/EditServer.php (1)
app/Filament/Components/Forms/Fields/StartupVariable.php (2)
StartupVariable(17-181)fromRecord(70-79)
🔇 Additional comments (3)
app/Filament/Admin/Resources/ServerResource/Pages/EditServer.php (1)
10-10: Good switch to the custom StartupVariable field.Import looks correct and aligns this page with the new component used elsewhere in the PR.
app/Filament/Components/Forms/Fields/StartupVariable.php (2)
150-163: Type inference looks good; consider future-proofing boolean/enum cases.Current checks are fine. If eggs emit boolean as “in:true,false” rather than “boolean”, getType() will still return Select (which is acceptable). Just a note that getSelectOptions() will then contain “true/false” strings and you’ll want to cast on dehydrate if you later store booleans.
Do you want me to add a small dehydrateStateUsing to coerce "true"/"false" strings to booleans only when getType() === Toggle?
59-79: fromForm()/fromRecord() APIs are solid.The closures are typed and will evaluate correctly within a relationship repeater context. Good separation for Create vs Edit flows.
Also closes #1602