[#15] Added option groups, separators and disabled options.#27
Open
AlexSkrypnyk wants to merge 8 commits into
Open
[#15] Added option groups, separators and disabled options.#27AlexSkrypnyk wants to merge 8 commits into
AlexSkrypnyk wants to merge 8 commits into
Conversation
📝 WalkthroughWalkthroughChoice lists now support headings, separators, disabled options, disabled reasons, selectable-value validation, filtered schemas, theme-driven rendering, and navigation that skips non-selectable rows across select, search, and multiselect widgets. ChangesChoice-list option support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant FieldBuilder
participant Field
participant WidgetFactory
participant ChoiceWidget
participant Theme
FieldBuilder->>Field: build ordered option rows
Field->>WidgetFactory: provide options and selectable values
WidgetFactory->>ChoiceWidget: initialize widget options
ChoiceWidget->>Theme: render headings, separators, and disabled labels
ChoiceWidget-->>Field: return selectable answer values
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Engine/Engine.php`:
- Around line 119-123: Reject non-array values for multi-choice fields before
validation: in the Engine validation logic around optionError(), detect
MultiSelect/MultiSearch fields whose value is not an array and throw
EngineException; alternatively update Field::optionError() to return an error
for non-array multi values instead of treating them as empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8185b839-0c4e-4d3b-992b-0d484d19306c
📒 Files selected for processing (28)
README.mdsrc/Builder/FieldBuilder.phpsrc/Config/Field.phpsrc/Config/FieldType.phpsrc/Config/Option.phpsrc/Config/OptionKind.phpsrc/Engine/Engine.phpsrc/Schema/SchemaGenerator.phpsrc/Schema/SchemaValidator.phpsrc/Theme/DefaultTheme.phpsrc/Theme/ThemeInterface.phpsrc/Widget/ChoiceListTrait.phpsrc/Widget/MultiSelectWidget.phpsrc/Widget/SearchWidget.phpsrc/Widget/SelectWidget.phpsrc/Widget/WidgetFactory.phptests/phpunit/Traits/MixedOptionsTrait.phptests/phpunit/Unit/Builder/FormTest.phptests/phpunit/Unit/Config/OptionTest.phptests/phpunit/Unit/Engine/EngineTest.phptests/phpunit/Unit/Schema/SchemaGeneratorTest.phptests/phpunit/Unit/Schema/SchemaValidatorTest.phptests/phpunit/Unit/Theme/ThemeTest.phptests/phpunit/Unit/Widget/MultiSearchWidgetTest.phptests/phpunit/Unit/Widget/MultiSelectWidgetTest.phptests/phpunit/Unit/Widget/SearchWidgetTest.phptests/phpunit/Unit/Widget/SelectWidgetTest.phptests/phpunit/Unit/Widget/WidgetFactoryTest.php
AlexSkrypnyk
force-pushed
the
feature/15-option-groups
branch
from
b06face to
8f376df
Compare
This comment has been minimized.
This comment has been minimized.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #27 +/- ##
==========================================
+ Coverage 99.39% 99.51% +0.12%
==========================================
Files 68 70 +2
Lines 1977 2082 +105
==========================================
+ Hits 1965 2072 +107
+ Misses 12 10 -2 ☔ View full report in Codecov by Harness. |
AlexSkrypnyk
force-pushed
the
feature/15-option-groups
branch
from
a29f0af to
698b972
Compare
This comment has been minimized.
This comment has been minimized.
AlexSkrypnyk
force-pushed
the
feature/15-option-groups
branch
from
698b972 to
ff56d7c
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Config/Field.php`:
- Around line 145-169: Update Field::optionError to explicitly reject non-scalar
items in multi-select values with a clear malformed-item error before calling
scalarOptionError; do not coerce arrays or objects to an empty string. Preserve
the existing scalar validation and return the first relevant error.
In `@src/Widget/SelectWidget.php`:
- Around line 93-112: Extract the shared heading/separator/disabled dispatch
from SelectWidget::view(), SearchWidget::view(), and MultiSelectWidget::view()
into a protected renderRows() helper in ChoiceListTrait. Have it accept the
theme, rows, and a closure for normal leaf rendering, preserving cursor and
disabled arguments, then update all three views to use the helper while
retaining their widget-specific leaf renderers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9ebfb611-47ea-4eee-8756-86b726325c77
⛔ Files ignored due to path filters (8)
docs/assets/widget-multiselect-groups-ascii-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-multiselect-groups-ascii.svgis excluded by!**/*.svgdocs/assets/widget-multiselect-groups-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-multiselect-groups.svgis excluded by!**/*.svgdocs/assets/widget-select-groups-ascii-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-select-groups-ascii.svgis excluded by!**/*.svgdocs/assets/widget-select-groups-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-select-groups.svgis excluded by!**/*.svg
📒 Files selected for processing (32)
README.mddocs/util/update-assets.phpplayground/3-widgets/widget-multiselect-groups.phpplayground/3-widgets/widget-select-groups.phpplayground/README.mdsrc/Builder/FieldBuilder.phpsrc/Config/Field.phpsrc/Config/FieldType.phpsrc/Config/Option.phpsrc/Config/OptionKind.phpsrc/Engine/Engine.phpsrc/Schema/SchemaGenerator.phpsrc/Schema/SchemaValidator.phpsrc/Theme/DefaultTheme.phpsrc/Theme/ThemeInterface.phpsrc/Widget/ChoiceListTrait.phpsrc/Widget/MultiSelectWidget.phpsrc/Widget/SearchWidget.phpsrc/Widget/SelectWidget.phpsrc/Widget/WidgetFactory.phptests/phpunit/Traits/MixedOptionsTrait.phptests/phpunit/Unit/Builder/FormTest.phptests/phpunit/Unit/Config/OptionTest.phptests/phpunit/Unit/Engine/EngineTest.phptests/phpunit/Unit/Schema/SchemaGeneratorTest.phptests/phpunit/Unit/Schema/SchemaValidatorTest.phptests/phpunit/Unit/Theme/ThemeTest.phptests/phpunit/Unit/Widget/MultiSearchWidgetTest.phptests/phpunit/Unit/Widget/MultiSelectWidgetTest.phptests/phpunit/Unit/Widget/SearchWidgetTest.phptests/phpunit/Unit/Widget/SelectWidgetTest.phptests/phpunit/Unit/Widget/WidgetFactoryTest.php
Comment on lines
+145
to
+169
| public function optionError(mixed $value): ?string { | ||
| if (!$this->type->constrainsToOptions() || $this->options === []) { | ||
| return NULL; | ||
| } | ||
|
|
||
| if ($this->type->isMulti()) { | ||
| if (!is_array($value)) { | ||
| return 'value must be a list'; | ||
| } | ||
|
|
||
| $items = $value; | ||
| } | ||
| else { | ||
| $items = [$value]; | ||
| } | ||
|
|
||
| foreach ($items as $item) { | ||
| $error = $this->scalarOptionError(is_scalar($item) ? (string) $item : ''); | ||
| if ($error !== NULL) { | ||
| return $error; | ||
| } | ||
| } | ||
|
|
||
| return NULL; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Non-scalar multi-select items are silently coerced to an empty-string error instead of a clear message.
is_scalar($item) ? (string) $item : '' turns a malformed item (nested array/object from a --prompts JSON payload) into '', producing a confusing value "" is not one of: ... error rather than clearly naming the malformed item. This undercuts the PR goal of rejecting bad headless input "with clear errors."
🐛 Proposed fix
foreach ($items as $item) {
- $error = $this->scalarOptionError(is_scalar($item) ? (string) $item : '');
+ if (!is_scalar($item)) {
+ return 'value must be a list of scalars';
+ }
+
+ $error = $this->scalarOptionError((string) $item);
if ($error !== NULL) {
return $error;
}
}📝 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.
Suggested change
| public function optionError(mixed $value): ?string { | |
| if (!$this->type->constrainsToOptions() || $this->options === []) { | |
| return NULL; | |
| } | |
| if ($this->type->isMulti()) { | |
| if (!is_array($value)) { | |
| return 'value must be a list'; | |
| } | |
| $items = $value; | |
| } | |
| else { | |
| $items = [$value]; | |
| } | |
| foreach ($items as $item) { | |
| $error = $this->scalarOptionError(is_scalar($item) ? (string) $item : ''); | |
| if ($error !== NULL) { | |
| return $error; | |
| } | |
| } | |
| return NULL; | |
| } | |
| public function optionError(mixed $value): ?string { | |
| if (!$this->type->constrainsToOptions() || $this->options === []) { | |
| return NULL; | |
| } | |
| if ($this->type->isMulti()) { | |
| if (!is_array($value)) { | |
| return 'value must be a list'; | |
| } | |
| $items = $value; | |
| } | |
| else { | |
| $items = [$value]; | |
| } | |
| foreach ($items as $item) { | |
| if (!is_scalar($item)) { | |
| return 'value must be a list of scalars'; | |
| } | |
| $error = $this->scalarOptionError((string) $item); | |
| if ($error !== NULL) { | |
| return $error; | |
| } | |
| } | |
| return NULL; | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 157-159: The method optionError uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Config/Field.php` around lines 145 - 169, Update Field::optionError to
explicitly reject non-scalar items in multi-select values with a clear
malformed-item error before calling scalarOptionError; do not coerce arrays or
objects to an empty string. Preserve the existing scalar validation and return
the first relevant error.
Comment on lines
+93
to
+112
| foreach ($this->options as $index => $option) { | ||
| if ($option->kind === OptionKind::Heading) { | ||
| $lines[] = $this->renderHeadingRow($theme, $option); | ||
|
|
||
| continue; | ||
| } | ||
|
|
||
| if ($option->kind === OptionKind::Separator) { | ||
| $lines[] = $this->renderSeparatorRow($theme); | ||
|
|
||
| continue; | ||
| } | ||
|
|
||
| if ($option->disabled) { | ||
| $lines[] = $theme->radio(FALSE) . ' ' . $this->renderDisabledLabel($theme, $option); | ||
|
|
||
| continue; | ||
| } | ||
|
|
||
| $lines[] = $this->renderRadioRow($theme, $option->label, $index === $this->cursor); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate render-dispatch loop across widgets.
This heading/separator/disabled/leaf-row loop is duplicated near-verbatim in SearchWidget::view() (same structure, filtered source) and, with a different leaf renderer, in MultiSelectWidget::view(). Any future fix (e.g. a new OptionKind, a rendering tweak) needs to be applied in three places consistently.
Consider hoisting the heading/separator/disabled dispatch into ChoiceListTrait as a shared helper that takes a closure for the "normal" leaf row:
♻️ Suggested extraction into `ChoiceListTrait`
protected function renderRows(ThemeInterface $theme, array $rows, \Closure $renderRow): array {
$lines = [];
foreach ($rows as $index => $option) {
if ($option->kind === OptionKind::Heading) {
$lines[] = $this->renderHeadingRow($theme, $option);
continue;
}
if ($option->kind === OptionKind::Separator) {
$lines[] = $this->renderSeparatorRow($theme);
continue;
}
if ($option->disabled) {
$lines[] = $renderRow($option, FALSE, TRUE);
continue;
}
$lines[] = $renderRow($option, $index === $this->cursor, FALSE);
}
return $lines;
}- public function view(ThemeInterface $theme): string {
- $lines = [];
-
- foreach ($this->options as $index => $option) {
- if ($option->kind === OptionKind::Heading) {
- $lines[] = $this->renderHeadingRow($theme, $option);
-
- continue;
- }
-
- if ($option->kind === OptionKind::Separator) {
- $lines[] = $this->renderSeparatorRow($theme);
-
- continue;
- }
-
- if ($option->disabled) {
- $lines[] = $theme->radio(FALSE) . ' ' . $this->renderDisabledLabel($theme, $option);
-
- continue;
- }
-
- $lines[] = $this->renderRadioRow($theme, $option->label, $index === $this->cursor);
- }
-
- return implode("\n", $lines);
- }
+ public function view(ThemeInterface $theme): string {
+ return implode("\n", $this->renderRows($theme, $this->options, fn (Option $option, bool $current, bool $disabled): string => $disabled
+ ? $theme->radio(FALSE) . ' ' . $this->renderDisabledLabel($theme, $option)
+ : $this->renderRadioRow($theme, $option->label, $current)));
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Widget/SelectWidget.php` around lines 93 - 112, Extract the shared
heading/separator/disabled dispatch from SelectWidget::view(),
SearchWidget::view(), and MultiSelectWidget::view() into a protected
renderRows() helper in ChoiceListTrait. Have it accept the theme, rows, and a
closure for normal leaf rendering, preserving cursor and disabled arguments,
then update all three views to use the helper while retaining their
widget-specific leaf renderers.
ff56d7c to
d7283ec
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Builder/FieldBuilder.php`:
- Around line 366-415: Update Toggle default resolution in resolveDefault() to
skip headings, separators, and disabled options by selecting the first option
for which selectable() is true. Return that option’s value, and retain the
existing type default when no selectable option exists, keeping
WidgetFactory::labels() and Toggle defaults aligned.
In `@src/Config/Option.php`:
- Around line 29-43: In Option::__construct, stop promoting $disabledReason
directly; declare an explicit camelCase disabledReason property on the class,
change the constructor parameter to snake_case $disabled_reason, and assign it
to $this->disabledReason within the constructor while preserving the existing
default and behavior.
In `@src/Widget/MultiSelectWidget.php`:
- Around line 148-174: visible() filtering is duplicated between
MultiSelectWidget and SearchWidget. Add a shared protected filterVisible(string
$filter): array method to ChoiceListTrait containing the existing empty-filter,
case-insensitive label matching, and Option-kind logic, then update both
widgets’ visible() methods to delegate to it and remove the duplicated
implementation.
In `@src/Widget/WidgetFactory.php`:
- Line 33: Rename the constructor argument in WidgetFactory::__construct() to
snake_case, such as $external_editor_available, while retaining the camelCase
property name $externalEditorAvailable by removing promotion and assigning the
argument to the property within the constructor.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 16236db3-6e6a-4c79-9ab4-52802e779f0b
⛔ Files ignored due to path filters (8)
docs/assets/widget-multiselect-groups-ascii-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-multiselect-groups-ascii.svgis excluded by!**/*.svgdocs/assets/widget-multiselect-groups-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-multiselect-groups.svgis excluded by!**/*.svgdocs/assets/widget-select-groups-ascii-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-select-groups-ascii.svgis excluded by!**/*.svgdocs/assets/widget-select-groups-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-select-groups.svgis excluded by!**/*.svg
📒 Files selected for processing (32)
README.mddocs/util/update-assets.phpplayground/3-widgets/widget-multiselect-groups.phpplayground/3-widgets/widget-select-groups.phpplayground/README.mdsrc/Builder/FieldBuilder.phpsrc/Config/Field.phpsrc/Config/FieldType.phpsrc/Config/Option.phpsrc/Config/OptionKind.phpsrc/Engine/Engine.phpsrc/Schema/SchemaGenerator.phpsrc/Schema/SchemaValidator.phpsrc/Theme/DefaultTheme.phpsrc/Theme/ThemeInterface.phpsrc/Widget/ChoiceListTrait.phpsrc/Widget/MultiSelectWidget.phpsrc/Widget/SearchWidget.phpsrc/Widget/SelectWidget.phpsrc/Widget/WidgetFactory.phptests/phpunit/Traits/MixedOptionsTrait.phptests/phpunit/Unit/Builder/FormTest.phptests/phpunit/Unit/Config/OptionTest.phptests/phpunit/Unit/Engine/EngineTest.phptests/phpunit/Unit/Schema/SchemaGeneratorTest.phptests/phpunit/Unit/Schema/SchemaValidatorTest.phptests/phpunit/Unit/Theme/ThemeTest.phptests/phpunit/Unit/Widget/MultiSearchWidgetTest.phptests/phpunit/Unit/Widget/MultiSelectWidgetTest.phptests/phpunit/Unit/Widget/SearchWidgetTest.phptests/phpunit/Unit/Widget/SelectWidgetTest.phptests/phpunit/Unit/Widget/WidgetFactoryTest.php
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Builder/FieldBuilder.php`:
- Around line 366-415: Update Toggle default resolution in resolveDefault() to
skip headings, separators, and disabled options by selecting the first option
for which selectable() is true. Return that option’s value, and retain the
existing type default when no selectable option exists, keeping
WidgetFactory::labels() and Toggle defaults aligned.
In `@src/Config/Option.php`:
- Around line 29-43: In Option::__construct, stop promoting $disabledReason
directly; declare an explicit camelCase disabledReason property on the class,
change the constructor parameter to snake_case $disabled_reason, and assign it
to $this->disabledReason within the constructor while preserving the existing
default and behavior.
In `@src/Widget/MultiSelectWidget.php`:
- Around line 148-174: visible() filtering is duplicated between
MultiSelectWidget and SearchWidget. Add a shared protected filterVisible(string
$filter): array method to ChoiceListTrait containing the existing empty-filter,
case-insensitive label matching, and Option-kind logic, then update both
widgets’ visible() methods to delegate to it and remove the duplicated
implementation.
In `@src/Widget/WidgetFactory.php`:
- Line 33: Rename the constructor argument in WidgetFactory::__construct() to
snake_case, such as $external_editor_available, while retaining the camelCase
property name $externalEditorAvailable by removing promotion and assigning the
argument to the property within the constructor.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 16236db3-6e6a-4c79-9ab4-52802e779f0b
⛔ Files ignored due to path filters (8)
docs/assets/widget-multiselect-groups-ascii-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-multiselect-groups-ascii.svgis excluded by!**/*.svgdocs/assets/widget-multiselect-groups-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-multiselect-groups.svgis excluded by!**/*.svgdocs/assets/widget-select-groups-ascii-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-select-groups-ascii.svgis excluded by!**/*.svgdocs/assets/widget-select-groups-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-select-groups.svgis excluded by!**/*.svg
📒 Files selected for processing (32)
README.mddocs/util/update-assets.phpplayground/3-widgets/widget-multiselect-groups.phpplayground/3-widgets/widget-select-groups.phpplayground/README.mdsrc/Builder/FieldBuilder.phpsrc/Config/Field.phpsrc/Config/FieldType.phpsrc/Config/Option.phpsrc/Config/OptionKind.phpsrc/Engine/Engine.phpsrc/Schema/SchemaGenerator.phpsrc/Schema/SchemaValidator.phpsrc/Theme/DefaultTheme.phpsrc/Theme/ThemeInterface.phpsrc/Widget/ChoiceListTrait.phpsrc/Widget/MultiSelectWidget.phpsrc/Widget/SearchWidget.phpsrc/Widget/SelectWidget.phpsrc/Widget/WidgetFactory.phptests/phpunit/Traits/MixedOptionsTrait.phptests/phpunit/Unit/Builder/FormTest.phptests/phpunit/Unit/Config/OptionTest.phptests/phpunit/Unit/Engine/EngineTest.phptests/phpunit/Unit/Schema/SchemaGeneratorTest.phptests/phpunit/Unit/Schema/SchemaValidatorTest.phptests/phpunit/Unit/Theme/ThemeTest.phptests/phpunit/Unit/Widget/MultiSearchWidgetTest.phptests/phpunit/Unit/Widget/MultiSelectWidgetTest.phptests/phpunit/Unit/Widget/SearchWidgetTest.phptests/phpunit/Unit/Widget/SelectWidgetTest.phptests/phpunit/Unit/Widget/WidgetFactoryTest.php
🛑 Comments failed to post (4)
src/Builder/FieldBuilder.php (1)
366-415: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Choose the first selectable row for Toggle defaults.
These new APIs allow a heading, separator, or disabled option to precede the first selectable option, but
resolveDefault()still returnsreset($this->options)->valueat Lines 480–482. A Toggle can therefore start with an empty or disabled default even thoughWidgetFactory::labels()excludes that row.Update Toggle default resolution to return the first
selectable()option, falling back to the type default when none exists.Proposed fix
if ($this->fieldType === FieldType::Toggle && $this->options !== []) { - return reset($this->options)->value; + foreach ($this->options as $option) { + if ($option->selectable()) { + return $option->value; + } + } }📝 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.if ($this->fieldType === FieldType::Toggle && $this->options !== []) { foreach ($this->options as $option) { if ($option->selectable()) { return $option->value; } } }🧰 Tools
🪛 PHPMD (2.15.0)
[error] 374-374: The method option has a boolean flag argument $disabled, which is a certain sign of a Single Responsibility Principle violation. (undefined)
(BooleanArgumentFlag)
[error] 374-391: The parameter $disabled_reason is not named in camelCase. (undefined)
(CamelCaseParameterName)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Builder/FieldBuilder.php` around lines 366 - 415, Update Toggle default resolution in resolveDefault() to skip headings, separators, and disabled options by selecting the first option for which selectable() is true. Return that option’s value, and retain the existing type default when no selectable option exists, keeping WidgetFactory::labels() and Toggle defaults aligned.src/Config/Option.php (1)
29-43: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Separate the promoted property from its constructor argument.
$disabledReasonis both a class property and a constructor argument. The repository rules require camelCase properties but snake_case method arguments; declare the property explicitly and accept$disabled_reason, assigning it in the constructor.As per coding guidelines, method arguments use snake_case while class properties use camelCase.
Proposed fix
final readonly class Option { + public string $disabledReason; + public function __construct( public string $value, public string $label, public string $description = '', public OptionKind $kind = OptionKind::Option, public bool $disabled = FALSE, - public string $disabledReason = '', + string $disabled_reason = '', ) { + $this->disabledReason = $disabled_reason; }📝 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.public string $disabledReason; /** * `@param` \DrevOps\Tui\Config\OptionKind $kind * The row kind. * `@param` bool $disabled * Whether a selectable Option row is shown but cannot be selected. * `@param` string $disabled_reason * The reason shown beside a disabled option. */ public function __construct( public string $value, public string $label, public string $description = '', public OptionKind $kind = OptionKind::Option, public bool $disabled = FALSE, string $disabled_reason = '', ) { $this->disabledReason = $disabled_reason; }🧰 Tools
🪛 PHPMD (2.15.0)
[error] 41-41: The method __construct has a boolean flag argument $disabled, which is a certain sign of a Single Responsibility Principle violation. (undefined)
(BooleanArgumentFlag)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Config/Option.php` around lines 29 - 43, In Option::__construct, stop promoting $disabledReason directly; declare an explicit camelCase disabledReason property on the class, change the constructor parameter to snake_case $disabled_reason, and assign it to $this->disabledReason within the constructor while preserving the existing default and behavior.Source: Coding guidelines
src/Widget/MultiSelectWidget.php (1)
148-174: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
visible()is duplicated verbatim inSearchWidget.The filter-based
visible()implementation here is identical toSearchWidget::visible()(same kind check, same case-insensitivestr_containsmatch). Consider hoisting it intoChoiceListTraitas a sharedfilterVisible(string $filter): array(or similar) operating on$this->options, so future filtering tweaks (e.g. matching descriptions, fuzzy match) apply once instead of drifting across widgets.♻️ Suggested extraction into `ChoiceListTrait`
protected function filterVisible(string $filter): array { if ($filter === '') { return $this->options; } $needle = strtolower($filter); return array_values(array_filter($this->options, fn(Option $option): bool => $option->kind === OptionKind::Option && str_contains(strtolower($option->label), $needle))); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Widget/MultiSelectWidget.php` around lines 148 - 174, visible() filtering is duplicated between MultiSelectWidget and SearchWidget. Add a shared protected filterVisible(string $filter): array method to ChoiceListTrait containing the existing empty-filter, case-insensitive label matching, and Option-kind logic, then update both widgets’ visible() methods to delegate to it and remove the duplicated implementation.src/Widget/WidgetFactory.php (1)
33-33: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
$externalEditorAvailableargument should be snake_case.As per coding guidelines,
**/*.php: "Usesnake_casefor local variables and method arguments." This promoted constructor parameter is camelCase. Since the same identifier also serves as the class property (which must be camelCase), consider un-promoting it so the argument and property can each follow their respective convention.🔧 Suggested fix
- public function __construct(?KeyMap $keymap = NULL, protected bool $externalEditorAvailable = FALSE) { + public function __construct(?KeyMap $keymap = NULL, bool $external_editor_available = FALSE) { $this->keymap = $keymap ?? KeyMapManager::create(); + $this->externalEditorAvailable = $external_editor_available; }🧰 Tools
🪛 PHPMD (2.15.0)
[error] 33-33: The method __construct has a boolean flag argument $externalEditorAvailable, which is a certain sign of a Single Responsibility Principle violation. (undefined)
(BooleanArgumentFlag)
[warning] 33-33: Avoid excessively long variable names like $externalEditorAvailable. Keep variable name length under 20. (undefined)
(LongVariable)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Widget/WidgetFactory.php` at line 33, Rename the constructor argument in WidgetFactory::__construct() to snake_case, such as $external_editor_available, while retaining the camelCase property name $externalEditorAvailable by removing promotion and assigning the argument to the property within the constructor.Source: Coding guidelines
…option error format.
AlexSkrypnyk
force-pushed
the
feature/15-option-groups
branch
from
d7283ec to
179d09b
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/util/update-assets.php (1)
504-519: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThree near-identical job-registration loops - consider consolidating.
The
widgetInteractions()loop, the picker loop, and the new group-demos loop all repeat the same "for each flag variant, build awidget-<name><suffix>job with command/interact/rows/cols/at_needle" pattern. Extracting a shared helper (e.g.registerWidgetJobs(array &$jobs, string $name, string $interact, string $gate, int $rows, int $cols, array $flag_variants, string $project_dir)) would reduce duplication and prevent future drift (e.g. the inconsistentcolsvalues of 44 vs 60 across these blocks).Also applies to: 546-559, 569-581
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/util/update-assets.php` around lines 504 - 519, Consolidate the duplicated flag-variant job registration logic from the widgetInteractions loop and the picker and group-demos loops into a shared helper, such as registerWidgetJobs. Have it accept the jobs reference, name, interaction, gate, row and column counts, flag variants, and project directory, then build each job consistently; replace all three loops with calls to this helper while preserving their intended values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Widget/MultiSelectWidget.php`:
- Around line 148-156: Move the duplicated filtering logic from
MultiSelectWidget::visible() and SearchWidget::visible() into a shared protected
filterOptions(string $filter): array method on ChoiceListTrait, preserving the
current empty-filter and case-insensitive label-matching behavior; update both
visible() methods to delegate to this helper.
In `@src/Widget/SearchWidget.php`:
- Around line 111-119: Extract the shared filtering logic from
SearchWidget::visible() and MultiSelectWidget::visible() into a
visibleRows(array $rows, string $filter): array helper in ChoiceListTrait.
Update both visible() methods to delegate to this helper while preserving the
empty-filter behavior and Option-kind substring matching.
In `@tests/phpunit/Unit/Widget/SearchWidgetTest.php`:
- Around line 24-25: Remove the redundant #[CoversClass(ChoiceListTrait::class)]
attribute from SearchWidgetTest, leaving SearchWidget::class as the sole
coverage target; this matches PHPUnit 11.5.55 behavior and documentation, where
covered classes include their used traits.
In `@tests/phpunit/Unit/Widget/SelectWidgetTest.php`:
- Around line 30-31: Remove the redundant #[CoversClass(ChoiceListTrait::class)]
attribute from SelectWidgetTest.php, keeping #[CoversClass(SelectWidget::class)]
as the coverage declaration for the trait used by SelectWidget.
---
Outside diff comments:
In `@docs/util/update-assets.php`:
- Around line 504-519: Consolidate the duplicated flag-variant job registration
logic from the widgetInteractions loop and the picker and group-demos loops into
a shared helper, such as registerWidgetJobs. Have it accept the jobs reference,
name, interaction, gate, row and column counts, flag variants, and project
directory, then build each job consistently; replace all three loops with calls
to this helper while preserving their intended values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 77b63b39-b0d0-4b44-a61c-c4f95ff00e55
⛔ Files ignored due to path filters (8)
docs/assets/widget-multiselect-groups-ascii-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-multiselect-groups-ascii.svgis excluded by!**/*.svgdocs/assets/widget-multiselect-groups-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-multiselect-groups.svgis excluded by!**/*.svgdocs/assets/widget-select-groups-ascii-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-select-groups-ascii.svgis excluded by!**/*.svgdocs/assets/widget-select-groups-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-select-groups.svgis excluded by!**/*.svg
📒 Files selected for processing (32)
README.mddocs/util/update-assets.phpplayground/3-widgets/widget-multiselect-groups.phpplayground/3-widgets/widget-select-groups.phpplayground/README.mdsrc/Builder/FieldBuilder.phpsrc/Config/Field.phpsrc/Config/FieldType.phpsrc/Config/Option.phpsrc/Config/OptionKind.phpsrc/Engine/Engine.phpsrc/Schema/SchemaGenerator.phpsrc/Schema/SchemaValidator.phpsrc/Theme/DefaultTheme.phpsrc/Theme/ThemeInterface.phpsrc/Widget/ChoiceListTrait.phpsrc/Widget/MultiSelectWidget.phpsrc/Widget/SearchWidget.phpsrc/Widget/SelectWidget.phpsrc/Widget/WidgetFactory.phptests/phpunit/Traits/MixedOptionsTrait.phptests/phpunit/Unit/Builder/FormTest.phptests/phpunit/Unit/Config/OptionTest.phptests/phpunit/Unit/Engine/EngineTest.phptests/phpunit/Unit/Schema/SchemaGeneratorTest.phptests/phpunit/Unit/Schema/SchemaValidatorTest.phptests/phpunit/Unit/Theme/ThemeTest.phptests/phpunit/Unit/Widget/MultiSearchWidgetTest.phptests/phpunit/Unit/Widget/MultiSelectWidgetTest.phptests/phpunit/Unit/Widget/SearchWidgetTest.phptests/phpunit/Unit/Widget/SelectWidgetTest.phptests/phpunit/Unit/Widget/WidgetFactoryTest.php
Comment on lines
148
to
+156
| protected function visible(): array { | ||
| if ($this->filter === '') { | ||
| return $this->values; | ||
| return $this->options; | ||
| } | ||
|
|
||
| $needle = strtolower($this->filter); | ||
|
|
||
| return array_values(array_filter($this->values, fn(string $value): bool => str_contains(strtolower($this->labels[$value] ?? $value), $needle))); | ||
| return array_values(array_filter($this->options, fn(Option $option): bool => $option->kind === OptionKind::Option && str_contains(strtolower($option->label), $needle))); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate filter logic with SearchWidget::visible().
This visible() implementation is byte-for-byte identical to SearchWidget::visible() (per repo graph context). Since ChoiceListTrait already centralizes shared option-list behavior (initOptions, firstSelectable, stepCursor, render helpers), consider moving this filter into the trait so both widgets share one implementation instead of two copies that can silently drift.
♻️ Proposed refactor: hoist filtering into ChoiceListTrait
- protected function visible(): array {
- if ($this->filter === '') {
- return $this->options;
- }
-
- $needle = strtolower($this->filter);
-
- return array_values(array_filter($this->options, fn(Option $option): bool => $option->kind === OptionKind::Option && str_contains(strtolower($option->label), $needle)));
- }
+ protected function visible(): array {
+ return $this->filterOptions($this->filter);
+ }And in ChoiceListTrait:
protected function filterOptions(string $filter): array {
if ($filter === '') {
return $this->options;
}
$needle = strtolower($filter);
return array_values(array_filter($this->options, fn(Option $option): bool => $option->kind === OptionKind::Option && str_contains(strtolower($option->label), $needle)));
}Apply the same change to SearchWidget::visible().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Widget/MultiSelectWidget.php` around lines 148 - 156, Move the duplicated
filtering logic from MultiSelectWidget::visible() and SearchWidget::visible()
into a shared protected filterOptions(string $filter): array method on
ChoiceListTrait, preserving the current empty-filter and case-insensitive
label-matching behavior; update both visible() methods to delegate to this
helper.
Comment on lines
111
to
+119
| protected function visible(): array { | ||
| if ($this->filter === '') { | ||
| return $this->values; | ||
| return $this->options; | ||
| } | ||
|
|
||
| $needle = strtolower($this->filter); | ||
|
|
||
| return array_values(array_filter($this->values, fn(string $value): bool => str_contains(strtolower($this->labels[$value] ?? $value), $needle))); | ||
| return array_values(array_filter($this->options, fn(Option $option): bool => $option->kind === OptionKind::Option && str_contains(strtolower($option->label), $needle))); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate filtering logic — could move into ChoiceListTrait.
visible() here is functionally identical to MultiSelectWidget::visible() (same Option-kind + substring-match predicate). ChoiceListTrait already centralizes firstSelectable/stepCursor/render helpers shared by these widgets; extracting a visibleRows(array $rows, string $filter): array helper there would remove this duplication and keep future filtering tweaks (e.g. matching on value too) in one place.
♻️ Proposed extraction into ChoiceListTrait
+ /**
+ * Rows matching a case-insensitive label filter (Option rows only).
+ *
+ * `@param` list<\DrevOps\Tui\Config\Option> $rows
+ * The declared rows.
+ * `@param` string $filter
+ * The current filter text; empty returns every row unchanged.
+ *
+ * `@return` list<\DrevOps\Tui\Config\Option>
+ * The visible rows.
+ */
+ protected function visibleRows(array $rows, string $filter): array {
+ if ($filter === '') {
+ return $rows;
+ }
+
+ $needle = strtolower($filter);
+
+ return array_values(array_filter($rows, fn(Option $option): bool => $option->kind === OptionKind::Option && str_contains(strtolower($option->label), $needle)));
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Widget/SearchWidget.php` around lines 111 - 119, Extract the shared
filtering logic from SearchWidget::visible() and MultiSelectWidget::visible()
into a visibleRows(array $rows, string $filter): array helper in
ChoiceListTrait. Update both visible() methods to delegate to this helper while
preserving the empty-filter behavior and Option-kind substring matching.
Comment on lines
+24
to
25
| #[CoversClass(ChoiceListTrait::class)] | ||
| #[Group('widget')] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Redundant #[CoversClass(ChoiceListTrait::class)].
PHPUnit's #[CoversClass] already attributes coverage for the traits used by the targeted class, so SearchWidget::class alone covers ChoiceListTrait. PHPUnit's own coverage docs explicitly advise against targeting traits this way.
🧹 Proposed fix
#[CoversClass(SearchWidget::class)]
-#[CoversClass(ChoiceListTrait::class)]
#[Group('widget')]
final class SearchWidgetTest extends TestCase {Please confirm against the PHPUnit version pinned in this repo (11.5.55 per the library context) that #[CoversClass] on a class already attributes coverage for its traits, as documented for PHPUnit 11.x/12.x.
📝 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.
Suggested change
| #[CoversClass(ChoiceListTrait::class)] | |
| #[Group('widget')] | |
| #[CoversClass(SearchWidget::class)] | |
| #[Group('widget')] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/phpunit/Unit/Widget/SearchWidgetTest.php` around lines 24 - 25, Remove
the redundant #[CoversClass(ChoiceListTrait::class)] attribute from
SearchWidgetTest, leaving SearchWidget::class as the sole coverage target; this
matches PHPUnit 11.5.55 behavior and documentation, where covered classes
include their used traits.
Comment on lines
+30
to
31
| #[CoversClass(ChoiceListTrait::class)] | ||
| #[Group('widget')] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial
Redundant #[CoversClass(ChoiceListTrait::class)].
Same as flagged in SearchWidgetTest.php: #[CoversClass(SelectWidget::class)] already attributes coverage for ChoiceListTrait, since SelectWidget uses it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/phpunit/Unit/Widget/SelectWidgetTest.php` around lines 30 - 31, Remove
the redundant #[CoversClass(ChoiceListTrait::class)] attribute from
SelectWidgetTest.php, keeping #[CoversClass(SelectWidget::class)] as the
coverage declaration for the trait used by SelectWidget.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #15
Summary
Adds group headings, non-selectable separators, and disabled options (with an optional reason) to the choice widgets (
select,multiselect,search, andmultisearchby inheritance). Non-selectable rows are visual only: navigation skips them, they can never be highlighted or selected, they never appear in the collected value, and they are excluded from the JSON schema. Headless collection rejects a supplied disabled - or otherwise unknown - option value with a clear, value-naming error.Changes
Option model (
src/Config/)OptionKindenum (Option,Separator,Heading).Optiongainskind,disabled,disabledReason, aselectable()helper, and a staticOption::list()normalizer that accepts either the['value' => 'label']map shorthand or alist<Option>.Field::$optionsis now an orderedlist<Option>; addsselectableValues()andoptionError()(rejects a non-array multi value, a disabled value, or an unknown value, naming the offending item).FieldTypegainsconstrainsToOptions()andisMulti().Builder (
src/Builder/FieldBuilder.php)option()gainsdisabledanddisabled_reasonparameters, and repeated option values dedupe in place.separator()andheading()methods add the non-selectable rows.Widgets (
src/Widget/)ChoiceListTraitcentralizes selectable-skipping navigation and heading/separator/disabled rendering.SelectWidget,SearchWidgetandMultiSelectWidget(plusMultiSearchWidgetby inheritance) takelist<Option>, skip non-selectable rows, and refuse to select a disabled option; a filter shows only options (headings and separators drop away).SuggestWidgetreceives selectable values only.Theme (
src/Theme/)heading(),divider()anddisabled()atoms onThemeInterfaceandDefaultTheme, so every kind is theme-driven.Headless (
src/Engine/,src/Schema/)Engine::collect()andSchemaValidatorreject a disabled/unknown supplied value through the sharedField::optionError();SchemaGeneratoremits selectable options only.Docs, playground and screenshots
selectand a groupedmultiselect) rendered through the project's existing asset generator.widget-select-groups.phpandwidget-multiselect-groups.phpplayground demos, registered as generator jobs.Tests
OptionTest, a sharedMixedOptionsTrait, and cases across the widget, engine, schema, theme and builder suites. The full suite passes (714 tests) and lint is clean.Before / After