[#9] Added a date picker calendar widget.#32
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (8)
📒 Files selected for processing (28)
📝 WalkthroughWalkthroughAdds a ChangesDate picker feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant WidgetFactory
participant DateWidget
participant DateBounds
User->>WidgetFactory: create date field widget
WidgetFactory->>DateWidget: seed value and bounds
User->>DateWidget: navigate calendar
DateWidget->>DateBounds: clamp cursor
User->>DateWidget: confirm selection
DateWidget-->>User: return ISO date
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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 #32 +/- ##
==========================================
+ Coverage 99.55% 99.58% +0.02%
==========================================
Files 73 76 +3
Lines 2272 2411 +139
==========================================
+ Hits 2262 2401 +139
Misses 10 10 ☔ View full report in Codecov by Harness. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Engine/Engine.php (1)
190-204: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject malformed
FieldType::Dateinput inEngine::validateValue()
Tui::collect()feeds raw--prompts/env values intoEngine::collect(), and this path only checksdateBounds;DateBounds::violation()returnsNULLon parse failure, so bad date strings can still be accepted unchanged. Add a strict date parse/type check here.🤖 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/Engine/Engine.php` around lines 190 - 204, The validateValue() method must reject malformed FieldType::Date inputs before applying date bounds. Add a strict type check and date parse validation for fields whose type is FieldType::Date, returning an appropriate validation error when the value is not a valid date; preserve the existing bounds and custom validator behavior for other field types.
🤖 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/Schema/SchemaGenerator.php`:
- Around line 53-55: In the schema-generation method, serialize dateBounds
weekStart consistently with other enums by replacing the weekStart ->name access
with ->value, unless the schema explicitly requires the enum case name rather
than its backing value.
---
Outside diff comments:
In `@src/Engine/Engine.php`:
- Around line 190-204: The validateValue() method must reject malformed
FieldType::Date inputs before applying date bounds. Add a strict type check and
date parse validation for fields whose type is FieldType::Date, returning an
appropriate validation error when the value is not a valid date; preserve the
existing bounds and custom validator behavior for other field types.
🪄 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: e73611b6-6658-414e-8cc7-42beb57aeb45
⛔ Files ignored due to path filters (8)
docs/assets/widget-date-ascii-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-date-ascii.svgis excluded by!**/*.svgdocs/assets/widget-date-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-date.svgis excluded by!**/*.svgdocs/assets/widgets-ascii-no-ansi.svgis excluded by!**/*.svgdocs/assets/widgets-ascii.svgis excluded by!**/*.svgdocs/assets/widgets-no-ansi.svgis excluded by!**/*.svgdocs/assets/widgets.svgis excluded by!**/*.svg
📒 Files selected for processing (27)
README.mddocs/util/update-assets.phpplayground/3-widgets/show.phpplayground/3-widgets/widget-date.phpplayground/3-widgets/widgets.phpsrc/Builder/FieldBuilder.phpsrc/Builder/PanelBuilder.phpsrc/Config/DateBounds.phpsrc/Config/Field.phpsrc/Config/FieldType.phpsrc/Config/Weekday.phpsrc/Engine/Engine.phpsrc/Schema/AgentHelp.phpsrc/Schema/SchemaGenerator.phpsrc/Schema/SchemaValidator.phpsrc/Widget/DateWidget.phpsrc/Widget/WidgetFactory.phptests/phpunit/Unit/Builder/FormTest.phptests/phpunit/Unit/Config/DateBoundsTest.phptests/phpunit/Unit/Config/WeekdayTest.phptests/phpunit/Unit/Engine/EngineDeclaredBehaviourTest.phptests/phpunit/Unit/Resolver/InputResolverTest.phptests/phpunit/Unit/Schema/AgentHelpTest.phptests/phpunit/Unit/Schema/SchemaGeneratorTest.phptests/phpunit/Unit/Schema/SchemaValidatorTest.phptests/phpunit/Unit/Widget/DateWidgetTest.phptests/phpunit/Unit/Widget/WidgetFactoryTest.php
This comment has been minimized.
This comment has been minimized.
778d723 to
e47aff7
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Schema/SchemaValidator.php (1)
116-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame bounds-precedence duplication as
Engine::validateValue().
checkBounds()andcheckDateBounds()are structurally identical aside from which bound property they read, and together they replicate the same "prefer number bounds, fall back to date bounds" precedence duplicated insrc/Engine/Engine.php(Line 196). Consolidating via a sharedField::boundsViolation()helper (mirroring the existingoptionError()precedent) would collapse both methods into one and keep the two validators from silently diverging.♻️ Proposed consolidation
- $bounds_error = $this->checkBounds($field, $value); - if ($bounds_error !== NULL) { - return $bounds_error; - } - - $date_error = $this->checkDateBounds($field, $value); - if ($date_error !== NULL) { - return $date_error; - } + $violation = $field->boundsViolation($value); + if ($violation !== NULL) { + return sprintf('Question "%s" must be %s.', $field->id, $violation); + }🤖 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/Schema/SchemaValidator.php` around lines 116 - 138, Consolidate bound selection into a shared Field::boundsViolation() helper, applying the existing precedence of numeric bounds before date bounds. Update SchemaValidator::checkBounds() and checkDateBounds() to use this helper, remove the duplicate date-specific method, and update Engine::validateValue() to use the same centralized logic so all validators remain consistent.src/Engine/Engine.php (1)
195-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider consolidating the bounds-precedence logic into
Field.This line duplicates the exact same "prefer number bounds, fall back to date bounds" precedence that
SchemaValidator::checkBounds()/checkDateBounds()implement separately (see src/Schema/SchemaValidator.php Lines 116-138).Fieldalready exposesoptionError()as a shared helper consumed by bothEngineandSchemaValidatorfor the analogous options-validation concern — the same pattern here would prevent the two call sites from silently diverging if the precedence or bounds types ever change.♻️ Proposed consolidation (requires adding a method to Field.php, not shown in this batch)
// In src/Config/Field.php public function boundsViolation(mixed $value): ?string { return $this->bounds?->violation($value) ?? $this->dateBounds?->violation($value); }protected function validateValue(Field $field, mixed $value): ?string { - $violation = $field->bounds?->violation($value) ?? $field->dateBounds?->violation($value); + $violation = $field->boundsViolation($value); if ($violation !== NULL) { return sprintf('must be %s.', $violation); }🤖 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/Engine/Engine.php` around lines 195 - 209, Consolidate bounds precedence in Field by adding a boundsViolation(mixed $value): ?string helper that prefers bounds over dateBounds. Update Engine::validateValue and SchemaValidator::checkBounds/checkDateBounds to call this shared method instead of duplicating null-coalescing logic, preserving the existing violation handling and messages.
🤖 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 `@README.md`:
- Around line 150-152: Update the README configuration example to import
DrevOps\Tui\Config\Weekday alongside the other imports, so the Weekday::Sunday
reference resolves when copied.
In `@src/Builder/FieldBuilder.php`:
- Around line 692-693: The parseBoundDate method currently accepts an
unrestricted string for the min/max selector; introduce a pure or backed enum
representing these two values, update parseBoundDate and related logic to accept
the enum, and pass the enum cases from the min/max calls around the affected
validation block. Use the enum’s value only when constructing exception
messages, including all branches covering the selector.
In `@src/Config/DateBounds.php`:
- Around line 30-35: Validate the bound ordering in the DateBounds constructor:
when both min and max are provided, reject cases where min is later than max,
using the class’s existing exception or validation convention. Keep null bounds
valid and ensure every direct DateBounds instantiation preserves the invariant
consumed by contains(), describe(), and clamp().
In `@src/Widget/WidgetFactory.php`:
- Line 60: The DateWidget currently hard-codes navigation keys instead of using
WidgetFactory’s injected keymap, so custom date bindings are ignored. Update
DateWidget::handle() and move() to resolve navigation through the configured
Action bindings, and ensure WidgetFactory passes that keymap when creating the
DateWidget; add a test covering remapped date navigation.
In `@tests/phpunit/Unit/Widget/DateWidgetTest.php`:
- Around line 201-212: Add symmetric max-bound coverage to
testDimsOutOfRangeDays by creating DateBounds with a max date and asserting a
day after that maximum is rendered with the theme description/dimmed styling,
while preserving the existing cursor-day assertion.
---
Outside diff comments:
In `@src/Engine/Engine.php`:
- Around line 195-209: Consolidate bounds precedence in Field by adding a
boundsViolation(mixed $value): ?string helper that prefers bounds over
dateBounds. Update Engine::validateValue and
SchemaValidator::checkBounds/checkDateBounds to call this shared method instead
of duplicating null-coalescing logic, preserving the existing violation handling
and messages.
In `@src/Schema/SchemaValidator.php`:
- Around line 116-138: Consolidate bound selection into a shared
Field::boundsViolation() helper, applying the existing precedence of numeric
bounds before date bounds. Update SchemaValidator::checkBounds() and
checkDateBounds() to use this helper, remove the duplicate date-specific method,
and update Engine::validateValue() to use the same centralized logic so all
validators remain consistent.
🪄 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: 3f35571a-5b1e-49b2-85df-a56813e95d9e
⛔ Files ignored due to path filters (8)
docs/assets/widget-date-ascii-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-date-ascii.svgis excluded by!**/*.svgdocs/assets/widget-date-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-date.svgis excluded by!**/*.svgdocs/assets/widgets-ascii-no-ansi.svgis excluded by!**/*.svgdocs/assets/widgets-ascii.svgis excluded by!**/*.svgdocs/assets/widgets-no-ansi.svgis excluded by!**/*.svgdocs/assets/widgets.svgis excluded by!**/*.svg
📒 Files selected for processing (27)
README.mddocs/util/update-assets.phpplayground/3-widgets/show.phpplayground/3-widgets/widget-date.phpplayground/3-widgets/widgets.phpsrc/Builder/FieldBuilder.phpsrc/Builder/PanelBuilder.phpsrc/Config/DateBounds.phpsrc/Config/Field.phpsrc/Config/FieldType.phpsrc/Config/Weekday.phpsrc/Engine/Engine.phpsrc/Schema/AgentHelp.phpsrc/Schema/SchemaGenerator.phpsrc/Schema/SchemaValidator.phpsrc/Widget/DateWidget.phpsrc/Widget/WidgetFactory.phptests/phpunit/Unit/Builder/FormTest.phptests/phpunit/Unit/Config/DateBoundsTest.phptests/phpunit/Unit/Config/WeekdayTest.phptests/phpunit/Unit/Engine/EngineDeclaredBehaviourTest.phptests/phpunit/Unit/Resolver/InputResolverTest.phptests/phpunit/Unit/Schema/AgentHelpTest.phptests/phpunit/Unit/Schema/SchemaGeneratorTest.phptests/phpunit/Unit/Schema/SchemaValidatorTest.phptests/phpunit/Unit/Widget/DateWidgetTest.phptests/phpunit/Unit/Widget/WidgetFactoryTest.php
This comment has been minimized.
This comment has been minimized.
075ee98 to
0a1ad0b
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Schema/SchemaValidator.php (1)
116-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMerge
checkBounds()/checkDateBounds()into a single range check.The two methods are identical except for the property accessed (
boundsvsdateBounds).Engine::validateValue()already coalesces the two bound sources with??— mirror that here to avoid duplicated logic and keep the two validators aligned as new bound types are added.♻️ Proposed refactor
- $bounds_error = $this->checkBounds($field, $value); - if ($bounds_error !== NULL) { - return $bounds_error; - } - - $date_error = $this->checkDateBounds($field, $value); - if ($date_error !== NULL) { - return $date_error; - } + $range_error = $this->checkRangeBounds($field, $value); + if ($range_error !== NULL) { + return $range_error; + } return $this->checkOptions($field, $value); } /** - * Check a number value against its declared bounds. + * Check a value against its declared number or date bounds. * * `@param` \DrevOps\Tui\Config\Field $field * The field. * `@param` mixed $value * The value. * * `@return` string|null * An error, or NULL when in range (or when the field declares no bounds). */ - protected function checkBounds(Field $field, mixed $value): ?string { - $violation = $field->bounds?->violation($value); - - return $violation === NULL ? NULL : sprintf('Question "%s" must be %s.', $field->id, $violation); - } - - /** - * Check a date value against its declared range. - * - * `@param` \DrevOps\Tui\Config\Field $field - * The field. - * `@param` mixed $value - * The value. - * - * `@return` string|null - * An error, or NULL when in range (or when the field declares no range). - */ - protected function checkDateBounds(Field $field, mixed $value): ?string { - $violation = $field->dateBounds?->violation($value); + protected function checkRangeBounds(Field $field, mixed $value): ?string { + $violation = $field->bounds?->violation($value) ?? $field->dateBounds?->violation($value); return $violation === NULL ? NULL : sprintf('Question "%s" must be %s.', $field->id, $violation); }Also applies to: 122-138
🤖 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/Schema/SchemaValidator.php` around lines 116 - 120, Merge checkBounds() and checkDateBounds() into one shared range-check method, selecting the bounds source with the same null-coalescing behavior as Engine::validateValue() (bounds ?? dateBounds). Update callers to use the unified method and remove the duplicated date-specific implementation, preserving the existing violation message and return behavior.
🤖 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 `@tests/phpunit/Unit/Schema/AgentHelpTest.php`:
- Around line 57-71: The testDateRangeAnnotation test should also cover
one-sided date bounds handled by dateNote(): add date fields configured with
minDate only and maxDate only, then assert the generated help contains the
corresponding “on or after” and “on or before” annotations. Preserve the
existing fully bounded and unbounded date assertions.
---
Outside diff comments:
In `@src/Schema/SchemaValidator.php`:
- Around line 116-120: Merge checkBounds() and checkDateBounds() into one shared
range-check method, selecting the bounds source with the same null-coalescing
behavior as Engine::validateValue() (bounds ?? dateBounds). Update callers to
use the unified method and remove the duplicated date-specific implementation,
preserving the existing violation message and return behavior.
🪄 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: e1e61890-07b6-4154-b340-58b995fef811
⛔ Files ignored due to path filters (8)
docs/assets/widget-date-ascii-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-date-ascii.svgis excluded by!**/*.svgdocs/assets/widget-date-no-ansi.svgis excluded by!**/*.svgdocs/assets/widget-date.svgis excluded by!**/*.svgdocs/assets/widgets-ascii-no-ansi.svgis excluded by!**/*.svgdocs/assets/widgets-ascii.svgis excluded by!**/*.svgdocs/assets/widgets-no-ansi.svgis excluded by!**/*.svgdocs/assets/widgets.svgis excluded by!**/*.svg
📒 Files selected for processing (28)
README.mddocs/util/update-assets.phpplayground/3-widgets/show.phpplayground/3-widgets/widget-date.phpplayground/3-widgets/widgets.phpsrc/Builder/FieldBuilder.phpsrc/Builder/PanelBuilder.phpsrc/Config/DateBounds.phpsrc/Config/Field.phpsrc/Config/FieldType.phpsrc/Config/Weekday.phpsrc/Engine/Engine.phpsrc/Input/VimKeyMap.phpsrc/Schema/AgentHelp.phpsrc/Schema/SchemaGenerator.phpsrc/Schema/SchemaValidator.phpsrc/Widget/DateWidget.phpsrc/Widget/WidgetFactory.phptests/phpunit/Unit/Builder/FormTest.phptests/phpunit/Unit/Config/DateBoundsTest.phptests/phpunit/Unit/Config/WeekdayTest.phptests/phpunit/Unit/Engine/EngineDeclaredBehaviourTest.phptests/phpunit/Unit/Resolver/InputResolverTest.phptests/phpunit/Unit/Schema/AgentHelpTest.phptests/phpunit/Unit/Schema/SchemaGeneratorTest.phptests/phpunit/Unit/Schema/SchemaValidatorTest.phptests/phpunit/Unit/Widget/DateWidgetTest.phptests/phpunit/Unit/Widget/WidgetFactoryTest.php
This comment has been minimized.
This comment has been minimized.
aa8a4c4 to
13fb51c
Compare
|
Closes #9
Summary
Adds a
datefield type: a month-calendar widget navigable by day, week and month, returning a normalized ISOYYYY-MM-DDstring. The widget is wired through the full stack - builder (minDate/maxDate/weekStart), config (DateBounds,Weekday), factory, engine validation, and JSON schema/agent-help - so the range and week-start declared on a field behave identically whether the value comes from the interactive TUI or headless collection. Navigation is routed through the configurable key map, like every other widget.Changes
src/Widget/DateWidget.phprenders a "Month YYYY" heading, a weekday header ordered from the configured week-start day, and a day grid. Day and week movement and Accept resolve through the injected key map - so the arrow keys, the vim preset'sh/j/k/land any consumer remap all reach them - while PageUp/PageDown change month and Home/End jump to the first/last day of the month. The cursor is always clamped into the declared range; out-of-range days stay visible but dimmed. The widget draws its own key-hint line (rendersHint()returnsTRUE), andplayground/3-widgets/widgets.phpskips the generic hint line for such widgets so the two can't contradict.src/Config/DateBounds.phpholds an optional inclusivemin/max(DateTimeImmutable) and aWeekday, and rejects a minimum declared after the maximum in its own constructor. It provides strictY-m-dparsing (round-tripped so a malformed or rolled-over value like2026-02-30is rejected rather than silently normalized),contains(),clamp(), a humandescribe()range phrase, and aviolation()check shared by the widget, engine and schema validator.src/Config/Weekday.phpis a new ISO-8601-backed enum (Monday = 1...Sunday = 7) withabbreviation(),fromDate(),sequence()(the seven weekdays in display order from a given start) andcolumnOf()(grid column placement).VimKeyMapbindsh/j/k/lto the date scope's move actions - safe because the calendar takes no typed input, like the single-choice list - so the vim preset drives the calendar the same way it drives the panel navigator.PanelBuilder::date()adds a date field;FieldBuilder::minDate(),maxDate()andweekStart()declare the range and first weekday, assembled into aDateBoundsonbuild()viaFieldBuilder::buildDateBounds(). An invalid bound string or a min declared after max throwsConfigException; the date setters are inert when called on a non-date field.Fieldgains adateBoundsproperty alongside the existingbounds;FieldType::Dateis a new enum case;WidgetFactoryseeds aDateWidgetfrom the field's current string value and declared bounds.Engine::validateValue()checksdateBounds?->violation()alongside the existing number-bounds check, so a--promptsor environment value outside the declared range is rejected with the same range phrase the interactive widget shows.SchemaValidatortype-checks date values (an empty string, or a strictY-m-ddate) and enforces the declared range;SchemaGeneratoremitsmin_date,max_dateandweek_starton the field's schema entry;AgentHelpappends a range annotation (orYYYY-MM-DDwhen unbounded) to a date field's help line.playground/3-widgets/widget-date.phpis a new standalone demo;widgets.phpandshow.phpinclude the date widget in the combined showcase;docs/util/update-assets.phpadds an expect script and row count for the date widget and regenerates the four combinedwidgets*.svgassets plus four new per-widgetwidget-date*.svgassets;README.mddocuments thedatefield (usage, bounds, key hints, screenshots) and updates the widget count to include it.DateBoundsTest,WeekdayTestandDateWidgetTestcover the added classes directly - navigation (including vim-preset remapping through an injected key map), bounds clamping and dimming on both edges, week-start layout, the constructor invariant, and boundary/impossible-date cases;FormTest,WidgetFactoryTest,EngineDeclaredBehaviourTest,InputResolverTest,SchemaGeneratorTest,SchemaValidatorTestandAgentHelpTestare extended to cover thedatefield type end to end. The full suite passes with 100% coverage on all new and touched classes.Before / After
Summary by CodeRabbit
YYYY-MM-DDoutput.