Dev#15
Merged
Merged
Conversation
…ode clarity - Updated write, writeError, writeRaw, and writeErrorRaw methods in ConsoleIO to use mixed type for messages. - Enhanced readability by adding new lines and consistent formatting in ConsoleIO, Colors, Fuzzy, Input, Key, RenderContext, Renderer, Shell, ShellResult, Spinner, SpinnerFrames, State, Terminal, Hooks, IOInterface, IPromptComponent, IRenderer, and NullIO. - Introduced a new filtered method in State for case-insensitive substring matching. - Added .editorconfig and .gitattributes for consistent coding standards and line endings. - Created phpunit.xml.dist and rector.php for testing and code quality tools. - Updated tests for consistency and clarity in various test files.
…Spinner, and their components - Introduced BufferIOUserInputsTest to validate user input handling in commands. - Added ShellTest to ensure shell command execution and output capture works correctly. - Created AlertTest to verify alert rendering for success, error, warning, and info messages. - Implemented RendererTest to check rendering logic and output for various states. - Developed SpinnerFramesTest and SpinnerTest to validate spinner frame functionality and behavior.
…update CoversClass attributes - Updated test classes (ColorsTest, FuzzyTest, HooksTest, InputTest, KeyTest, NullIOTest, RenderContextTest, RendererTest, ShellResultTest, SpinnerFramesTest, SpinnerTest, StateTest, TableTest) to use static function syntax for closures in various test methods. - Changed CoversClass attributes to use the class name directly instead of the fully qualified name. - Cleaned up formatting for better consistency across test files.
|
Warning Review limit reached
More reviews will be available in 50 minutes and 37 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughIntroduces ChangesKeyReader Abstraction
RadioGroup Component
Sequence Diagram(s)sequenceDiagram
participant Test as Test / Caller
participant AbstractPrompt
participant KeyReaderInterface
participant Component as Prompt Component
Test->>AbstractPrompt: withKeyReader(ArrayKeyReader)
Test->>AbstractPrompt: run()
AbstractPrompt->>KeyReaderInterface: setUp()
AbstractPrompt->>Component: setup()
loop engine loop
AbstractPrompt->>KeyReaderInterface: readKey()
KeyReaderInterface-->>AbstractPrompt: key string (or '' when exhausted)
alt key is ''
AbstractPrompt->>AbstractPrompt: break loop
else key is valid
AbstractPrompt->>Component: dispatch(key)
Component->>Component: update state
end
AbstractPrompt->>Component: render()
end
AbstractPrompt->>KeyReaderInterface: tearDown() [finally]
AbstractPrompt->>Component: resolve()
Component-->>AbstractPrompt: selected value
AbstractPrompt-->>Test: resolved value
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 5
🤖 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/AbstractPrompt.php`:
- Around line 72-75: The run() method in AbstractPrompt calls setUp() outside
the try block and tearDown() in the finally block after destroy()/dispatch(),
which means if setUp() or destroy()/dispatch() throws an exception, tearDown()
might not execute properly or at all. Restructure the run() method to ensure
tearDown() is always called as the final cleanup action regardless of where an
exception occurs, by moving setUp() inside the try block or wrapping the entire
operation with a try-finally that guarantees tearDown() executes after all other
operations (including destroy/dispatch).
In `@src/Components/RadioGroup.php`:
- Around line 40-43: The __construct method in the RadioGroup class does not
validate that the $choices array is non-empty, which causes runtime failures in
the UP/DOWN handlers that perform modulo operations and in range functions that
expect valid array sizes. Add validation in the constructor to ensure the
$choices array is not empty, throwing an appropriate exception if an empty array
is provided during construction.
- Around line 125-155: The render method in the RadioGroup class moves the
cursor up by the previous line count but only clears the lines as it writes new
content. When transitioning from a full options view to the done/collapsed view,
if the new frame has fewer lines than the previous one, leftover rows remain
visible on screen. After moving the cursor up by $this->lastLines, clear all
those previous rows before rendering the new content. Ensure that all
$this->lastLines rows are cleared regardless of how many lines the new frame
will contain, not just the lines being written.
In `@src/KeyReaderInterface.php`:
- Around line 26-29: The docblock comment in the KeyReaderInterface states that
returning an empty string is treated as a no-op key, but the actual
implementation in AbstractPrompt::run() treats an empty string as a stop signal
that exits the loop. Update the documentation comment in the KeyReaderInterface
to clarify that an empty string signals end-of-input and will terminate the
prompt loop, not act as a no-op. Also review and update any related
implementations such as ArrayKeyReader to ensure they align with this corrected
contract and any related documentation reflects the same behavior.
In `@tests/Unit/RadioGroupTest.php`:
- Around line 298-313: The test_render_no_ansi_when_colors_disabled() method
uses capture() which automatically strips ANSI codes, making it impossible to
detect if render() is still emitting color sequences when colors are disabled.
Instead of using capture(), call render() directly to get the raw output without
ANSI stripping, then assert that the output contains no SGR (Select Graphic
Rendition) color codes while keeping the existing assertions for the option text
content (Pick, small, medium, large, x-large).
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 604a24f6-4535-4ca3-8f54-3ccf3e13af9a
📒 Files selected for processing (9)
TODO.mdsrc/AbstractCommand.phpsrc/AbstractPrompt.phpsrc/Components/RadioGroup.phpsrc/Depends/ArrayKeyReader.phpsrc/Depends/TerminalKeyReader.phpsrc/KeyReaderInterface.phptests/Unit/KeyReaderTest.phptests/Unit/RadioGroupTest.php
Comment on lines
72
to
75
| public function run(): mixed | ||
| { | ||
| Terminal::enableRaw(); | ||
| $this->keyReader->setUp(); | ||
| $this->running = true; |
There was a problem hiding this comment.
Guarantee tearDown() execution on all failure paths.
setUp() is outside the try, and tearDown() runs after destroy()/dispatch() in the same finally. If setup or destroy throws, terminal cleanup can be skipped.
Suggested restructuring
public function run(): mixed
{
- $this->keyReader->setUp();
- $this->running = true;
-
try {
+ $this->keyReader->setUp();
+ $this->running = true;
+
$this->mount();
$this->dispatch('mount');
@@
- } catch (\Exception $e) {
+ } catch (\Exception $e) {
$this->handleError($e);
throw $e;
} finally {
- $this->destroy();
- $this->dispatch('destroy');
- $this->keyReader->tearDown();
+ try {
+ $this->destroy();
+ $this->dispatch('destroy');
+ } finally {
+ $this->keyReader->tearDown();
+ }
}
}Also applies to: 120-124
🤖 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/AbstractPrompt.php` around lines 72 - 75, The run() method in
AbstractPrompt calls setUp() outside the try block and tearDown() in the finally
block after destroy()/dispatch(), which means if setUp() or destroy()/dispatch()
throws an exception, tearDown() might not execute properly or at all.
Restructure the run() method to ensure tearDown() is always called as the final
cleanup action regardless of where an exception occurs, by moving setUp() inside
the try block or wrapping the entire operation with a try-finally that
guarantees tearDown() executes after all other operations (including
destroy/dispatch).
Comment on lines
+40
to
+43
| public function __construct( | ||
| private string $question, | ||
| private array $choices, | ||
| ) { |
There was a problem hiding this comment.
Guard against empty choice lists to avoid runtime crashes.
With [] choices, the UP/DOWN handlers do modulo by 0, which can throw at runtime; range(1, min(9, 0)) also produces unintended bindings. Enforce a non-empty invariant at construction.
💡 Suggested fix
public function __construct(
private string $question,
private array $choices,
) {
+ if ($choices === []) {
+ throw new \InvalidArgumentException('RadioGroup requires at least one choice.');
+ }
+
+ $this->choices = array_values($choices);
parent::__construct();
}Also applies to: 92-107
🤖 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/Components/RadioGroup.php` around lines 40 - 43, The __construct method
in the RadioGroup class does not validate that the $choices array is non-empty,
which causes runtime failures in the UP/DOWN handlers that perform modulo
operations and in range functions that expect valid array sizes. Add validation
in the constructor to ensure the $choices array is not empty, throwing an
appropriate exception if an empty array is provided during construction.
Comment on lines
+125
to
+155
| if ($this->lastLines > 0) { | ||
| Terminal::moveCursorUp($this->lastLines); | ||
| } | ||
|
|
||
| Terminal::hideCursor(); | ||
|
|
||
| $index = (int) $this->state->index; | ||
| $done = (bool) $this->state->done; | ||
| $lines = []; | ||
|
|
||
| // ── Line 1: question ────────────────────────────────── | ||
| $mark = $done | ||
| ? Colors::success('') | ||
| : Colors::wrap('? ', Colors::CYAN); | ||
| $lines[] = $mark . Colors::wrap($this->question, Colors::BOLD); | ||
|
|
||
| if (!$done) { | ||
| $lines = array_merge($lines, $this->renderOptions($index)); | ||
| $lines[] = Colors::muted(' ↑↓ move • 1-9 jump • ENTER confirm'); | ||
| } else { | ||
| $selected = $this->choices[$index] ?? ''; | ||
| $lines[] = Colors::wrap(' › ', Colors::GRAY) . Colors::wrap($selected, Colors::GREEN); | ||
| } | ||
|
|
||
| foreach ($lines as $line) { | ||
| Terminal::clearLine(); | ||
| echo $line . PHP_EOL; | ||
| } | ||
|
|
||
| $this->lastLines = count($lines); | ||
| } |
There was a problem hiding this comment.
Clear leftover rows when the rendered frame gets shorter.
When transitioning from the full options list to the done/collapsed view, old rows can remain visible because only the new line count is cleared/redrawn.
💡 Suggested fix
- foreach ($lines as $line) {
- Terminal::clearLine();
- echo $line . PHP_EOL;
- }
-
- $this->lastLines = count($lines);
+ $newLineCount = count($lines);
+ $rowsToRedraw = max($this->lastLines, $newLineCount);
+
+ for ($i = 0; $i < $rowsToRedraw; $i++) {
+ Terminal::clearLine();
+ echo ($lines[$i] ?? '') . PHP_EOL;
+ }
+
+ $this->lastLines = $newLineCount;🤖 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/Components/RadioGroup.php` around lines 125 - 155, The render method in
the RadioGroup class moves the cursor up by the previous line count but only
clears the lines as it writes new content. When transitioning from a full
options view to the done/collapsed view, if the new frame has fewer lines than
the previous one, leftover rows remain visible on screen. After moving the
cursor up by $this->lastLines, clear all those previous rows before rendering
the new content. Ensure that all $this->lastLines rows are cleared regardless of
how many lines the new frame will contain, not just the lines being written.
Comment on lines
+26
to
+29
| * The reader MUST block until a key is available, just as a real TTY does, | ||
| * so the event loop in AbstractPrompt::run() can yield correctly. | ||
| * Returning an empty string is treated as a no-op key (no bindings fire). | ||
| */ |
There was a problem hiding this comment.
Empty-string semantics in the public contract conflict with runtime behavior.
The interface says '' is a no-op, but AbstractPrompt::run() currently treats '' as end-of-input and exits the loop. Custom readers implemented against this contract can terminate prompts unexpectedly.
Please align the contract and implementation to one behavior (recommended: document '' as stop signal and update related docs, including ArrayKeyReader).
Proposed contract wording
- * Returning an empty string is treated as a no-op key (no bindings fire).
+ * Returning an empty string signals end-of-input and causes the prompt loop to stop.📝 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
| * The reader MUST block until a key is available, just as a real TTY does, | |
| * so the event loop in AbstractPrompt::run() can yield correctly. | |
| * Returning an empty string is treated as a no-op key (no bindings fire). | |
| */ | |
| * The reader MUST block until a key is available, just as a real TTY does, | |
| * so the event loop in AbstractPrompt::run() can yield correctly. | |
| * Returning an empty string signals end-of-input and causes the prompt loop to stop. | |
| */ |
🤖 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/KeyReaderInterface.php` around lines 26 - 29, The docblock comment in the
KeyReaderInterface states that returning an empty string is treated as a no-op
key, but the actual implementation in AbstractPrompt::run() treats an empty
string as a stop signal that exits the loop. Update the documentation comment in
the KeyReaderInterface to clarify that an empty string signals end-of-input and
will terminate the prompt loop, not act as a no-op. Also review and update any
related implementations such as ArrayKeyReader to ensure they align with this
corrected contract and any related documentation reflects the same behavior.
Comment on lines
+298
to
+313
| public function test_render_no_ansi_when_colors_disabled(): void | ||
| { | ||
| Colors::disable(); | ||
| $rg = new RadioGroup('Pick', self::CHOICES); | ||
| $this->mount($rg); | ||
|
|
||
| $output = $this->capture(fn() => $rg->render()); | ||
|
|
||
| $this->assertStringContainsString('Pick', $output); | ||
| $this->assertStringContainsString('small', $output); | ||
| $this->assertStringContainsString('medium', $output); | ||
| $this->assertStringContainsString('large', $output); | ||
| $this->assertStringContainsString('x-large', $output); | ||
|
|
||
|
|
||
| } |
There was a problem hiding this comment.
test_render_no_ansi_when_colors_disabled() currently can’t detect ANSI regressions.
capture() always strips ANSI, so this test will pass even if render still emits color sequences. Capture raw output for this case and assert no SGR color codes.
💡 Suggested fix
- $output = $this->capture(fn() => $rg->render());
+ $output = $this->capture(fn() => $rg->render(), stripAnsi: false);
$this->assertStringContainsString('Pick', $output);
$this->assertStringContainsString('small', $output);
$this->assertStringContainsString('medium', $output);
$this->assertStringContainsString('large', $output);
$this->assertStringContainsString('x-large', $output);
+ $this->assertDoesNotMatchRegularExpression('/\e\[[0-9;]*m/', $output);
@@
- private function capture(callable $fn): string
+ private function capture(callable $fn, bool $stripAnsi = true): string
{
ob_start();
$fn();
-
- return Colors::strip((string) ob_get_clean());
+ $output = (string) ob_get_clean();
+
+ return $stripAnsi ? Colors::strip($output) : $output;
}Also applies to: 377-383
🤖 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/Unit/RadioGroupTest.php` around lines 298 - 313, The
test_render_no_ansi_when_colors_disabled() method uses capture() which
automatically strips ANSI codes, making it impossible to detect if render() is
still emitting color sequences when colors are disabled. Instead of using
capture(), call render() directly to get the raw output without ANSI stripping,
then assert that the output contains no SGR (Select Graphic Rendition) color
codes while keeping the existing assertions for the option text content (Pick,
small, medium, large, x-large).
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
2 participants
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.
Description
Type of Change
Related Issues
Checklist
composer test)composer phpstan)CHANGELOG.mdentry is written (if user-facing)Testing
Screenshots / Terminal Output
Summary by CodeRabbit
New Features
Refactor
Tests