Conversation
📝 WalkthroughWalkthroughAdds a new DatePicker component to Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant DatePicker
participant ZagMachine
participant DOM
User->>App: Render DatePicker
App->>DatePicker: Mount & initialize
DatePicker->>ZagMachine: createMachine()
ZagMachine-->>DatePicker: machine/api
DatePicker->>DOM: Render template (input, trigger, popover)
DatePicker->>DatePicker: _renderGrid(view)
DatePicker->>DOM: Apply spread props to grid elements
User->>DOM: Click date cell
DOM->>ZagMachine: emit select
ZagMachine->>DatePicker: value change event
DatePicker->>App: call onValueChange(details)
DatePicker->>DOM: update rendered state
User->>DOM: Change view (month/year)
DOM->>ZagMachine: emit view change
ZagMachine->>DatePicker: update view
DatePicker->>DatePicker: _renderGrid(new view)
DatePicker->>DOM: update calendar display
App->>DatePicker: Unmount
DatePicker->>DatePicker: run _gridCleanups and dispose machine
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
aksuharun
force-pushed
the
feature/date-picker
branch
3 times, most recently
from
51d9c41 to
9c3c0f9
Compare
![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
🧹 Nitpick comments (2)
packages/gea-ui/src/components/date-picker.tsx (2)
10-13: Consider stronger typing forvalueproperty.Using
any[]loses type safety. The@zag-js/date-pickerlibrary provides aDateValuetype that could be used here for better IDE support and compile-time checks.+import type { DateValue } from '@zag-js/date-picker' + export default class DatePicker extends ZagComponent { declare open: boolean - declare value: any[] + declare value: DateValue[] declare valueAsString: string[] declare _gridCleanups: Array<() => void>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gea-ui/src/components/date-picker.tsx` around lines 10 - 13, Replace the loose any[] typing on the value property with the specific DateValue type from `@zag-js/date-picker` to restore type safety: import DateValue from '@zag-js/date-picker' (or the correct named export) and update the declaration for value to use DateValue (e.g., value: DateValue) while leaving valueAsString and _gridCleanups as-is; ensure the import is added at the top of the file and adjust any usages of the value property to satisfy the stronger type where necessary.
87-93: Grid re-renders on every spread application cycle.The
_applyAllSpreadsoverride calls_renderGridunconditionally, which clearsinnerHTMLand rebuilds the entire grid DOM. This happens on every Zag state change. For typical date picker usage this is acceptable, but for high-frequency updates this could cause performance overhead.Consider caching
api.viewand only re-rendering when the view actually changes, or when calendar data (api.weeks, grid data) differs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gea-ui/src/components/date-picker.tsx` around lines 87 - 93, The override _applyAllSpreads currently calls this._renderGrid every cycle; change it to only call _renderGrid when the visible view or calendar data actually changed by caching the previous view and/or weeks: store a local property like this._lastView (and optionally this._lastWeeksHash) and compare against this._api.view and this._api.weeks before invoking this._renderGrid(this._api); if different, call _renderGrid and update the cached values after render; otherwise skip the render to avoid clearing and rebuilding the DOM on every spread application.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/showcase/app.tsx`:
- Line 28: Revert any edits made to the example app file app.tsx: remove the
added/modified import or usage of DatePicker and undo changes at the other
referenced regions (around the blocks that correspond to lines near 293-301 and
615) so the examples/** files remain untouched; locate the DatePicker import and
any new code that references it and restore the original example content from
the upstream branch or discard your changes to that file so the example app is
exactly as in master.
In `@packages/gea-ui/tests/zag-bindings.test.ts`:
- Line 783: The test caches the DOM node in the variable content from view.el
once, which can become stale if the view swaps month/year; update the test to
re-query the content element from view.el immediately after any operation that
causes the view to switch (e.g., after navigation or state changes) instead of
using the previously cached content variable so assertions always run against
the current DOM node; look for the usages of content in this test file (the
initial query at const content = view.el?.querySelector('[data-part="content"]')
and the later assertions around where month/year switches occur) and replace
them with fresh queries or reassign content right after the switch.
- Line 731: Update the tests to use the describe/it style instead of test() by
changing the import from node:test to import { describe, it } from 'node:test'
and replacing each test('...', async () => { ... }) call with a describe('...',
() => { it('behaves as expected', async () => { ... }) }); specifically wrap the
existing test bodies (e.g., the "DatePicker: renders day grid, onValueChange
updates parent, view switching works" block) inside a describe block and move
the original test body into an it(...) block, preserving async/await, hooks, and
any assertions; ensure any top-level setup/teardown remains at the same scope
(use beforeEach/afterEach inside describe if needed) and import references to
test are removed.
---
Nitpick comments:
In `@packages/gea-ui/src/components/date-picker.tsx`:
- Around line 10-13: Replace the loose any[] typing on the value property with
the specific DateValue type from `@zag-js/date-picker` to restore type safety:
import DateValue from '@zag-js/date-picker' (or the correct named export) and
update the declaration for value to use DateValue (e.g., value: DateValue) while
leaving valueAsString and _gridCleanups as-is; ensure the import is added at the
top of the file and adjust any usages of the value property to satisfy the
stronger type where necessary.
- Around line 87-93: The override _applyAllSpreads currently calls
this._renderGrid every cycle; change it to only call _renderGrid when the
visible view or calendar data actually changed by caching the previous view
and/or weeks: store a local property like this._lastView (and optionally
this._lastWeeksHash) and compare against this._api.view and this._api.weeks
before invoking this._renderGrid(this._api); if different, call _renderGrid and
update the cached values after render; otherwise skip the render to avoid
clearing and rebuilding the DOM on every spread application.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 74d1b1b6-15dc-4eae-a631-1c2315cfd000
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
.changeset/add-date-picker.mddocs/gea-ui/interactive-components.mddocs/gea-ui/overview.mdexamples/showcase/app.tsxpackages/gea-ui/README.mdpackages/gea-ui/package.jsonpackages/gea-ui/src/components/date-picker.tsxpackages/gea-ui/src/index.tspackages/gea-ui/tests/zag-bindings.test.ts
| Accordion, | ||
| Collapsible, | ||
| Combobox, | ||
| DatePicker, |
There was a problem hiding this comment.
Avoid changes in examples/** per repo policy.
Lines 28, 293-301, and 615 modify showcase example app code, which is disallowed by the project rule for examples/**.
As per coding guidelines, examples/**: “Do not modify example app code under examples/. Example apps are integration tests for the framework.”
Also applies to: 293-301, 615-615
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/showcase/app.tsx` at line 28, Revert any edits made to the example
app file app.tsx: remove the added/modified import or usage of DatePicker and
undo changes at the other referenced regions (around the blocks that correspond
to lines near 293-301 and 615) so the examples/** files remain untouched; locate
the DatePicker import and any new code that references it and restore the
original example content from the upstream branch or discard your changes to
that file so the example app is exactly as in master.
|
|
||
| // ── DatePicker ──────────────────────────────────────────────────────────── | ||
|
|
||
| test('DatePicker: renders day grid, onValueChange updates parent, view switching works', async () => { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify test style in this file against repo guideline
rg -n "import test from 'node:test'|\\btest\\(" packages/gea-ui/tests/zag-bindings.test.ts
rg -n "import \\{[^}]*describe[^}]*it[^}]*\\} from 'node:test'" packages/gea-ui/tests/zag-bindings.test.tsRepository: dashersw/gea
Length of output: 857
Switch test blocks from test() to describe/it style.
The file imports and uses test() from node:test (line 15 and throughout), but the guideline requires describe/it pattern. Update the import to import { describe, it } from 'node:test' and convert all test(...) blocks to describe(..., () => { ... it(...) ... }) structure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/gea-ui/tests/zag-bindings.test.ts` at line 731, Update the tests to
use the describe/it style instead of test() by changing the import from
node:test to import { describe, it } from 'node:test' and replacing each
test('...', async () => { ... }) call with a describe('...', () => { it('behaves
as expected', async () => { ... }) }); specifically wrap the existing test
bodies (e.g., the "DatePicker: renders day grid, onValueChange updates parent,
view switching works" block) inside a describe block and move the original test
body into an it(...) block, preserving async/await, hooks, and any assertions;
ensure any top-level setup/teardown remains at the same scope (use
beforeEach/afterEach inside describe if needed) and import references to test
are removed.
| assert.ok(child._api, 'Zag API is connected') | ||
|
|
||
| // Day grid must be rendered inside [data-part="content"] | ||
| const content = view.el?.querySelector('[data-part="content"]') as HTMLElement |
There was a problem hiding this comment.
Re-query content after view switches to avoid false positives.
Line 783 caches content once. If month/year switches replace the content node, Lines 802-803 and 810-811 can still pass against a stale detached subtree.
🔧 Suggested test hardening
- const content = view.el?.querySelector('[data-part="content"]') as HTMLElement
- assert.ok(content, 'content element found')
- const dayTable = content.querySelector('table')
+ let content = view.el?.querySelector('[data-part="content"]') as HTMLElement
+ assert.ok(content, 'content element found')
+ const dayTable = content.querySelector('table')
assert.ok(dayTable, 'day view table is rendered')
@@
child._api.setView('month')
await flushMicrotasks()
assert.equal(child._api.view, 'month', 'view is month')
- const monthTable = content.querySelector('table')
+ content = view.el?.querySelector('[data-part="content"]') as HTMLElement
+ assert.ok(content, 'content element found after month switch')
+ const monthTable = content.querySelector('table')
assert.ok(monthTable, 'month view table is rendered')
@@
child._api.setView('year')
await flushMicrotasks()
assert.equal(child._api.view, 'year', 'view is year')
- const yearTable = content.querySelector('table')
+ content = view.el?.querySelector('[data-part="content"]') as HTMLElement
+ assert.ok(content, 'content element found after year switch')
+ const yearTable = content.querySelector('table')
assert.ok(yearTable, 'year view table is rendered')Also applies to: 802-803, 810-811
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/gea-ui/tests/zag-bindings.test.ts` at line 783, The test caches the
DOM node in the variable content from view.el once, which can become stale if
the view swaps month/year; update the test to re-query the content element from
view.el immediately after any operation that causes the view to switch (e.g.,
after navigation or state changes) instead of using the previously cached
content variable so assertions always run against the current DOM node; look for
the usages of content in this test file (the initial query at const content =
view.el?.querySelector('[data-part="content"]') and the later assertions around
where month/year switches occur) and replace them with fresh queries or reassign
content right after the switch.
There was a problem hiding this comment.
Pull request overview
Adds a new accessible DatePicker component to @geajs/ui (powered by @zag-js/date-picker), wires it into exports/showcase/docs, and introduces a regression test to validate Zag ↔ Gea state syncing.
Changes:
- Introduces
DatePickercomponent with day/month/year views and imperative grid rendering + cleanup. - Exports
DatePicker, documents it (README + docs), and adds it to the showcase (including footer component count fix). - Adds a zag-bindings regression test for
DatePicker(render, value updates, view switching).
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/gea-ui/src/components/date-picker.tsx | New DatePicker implementation using Zag machine + imperative grid rendering/cleanup |
| packages/gea-ui/src/index.ts | Exports DatePicker from the UI package entrypoint |
| packages/gea-ui/package.json | Adds @zag-js/date-picker dependency |
| package-lock.json | Locks @zag-js/date-picker (+ peers) into the workspace dependency graph |
| packages/gea-ui/tests/zag-bindings.test.ts | Adds regression test coverage for DatePicker bindings and lifecycle cleanup |
| packages/gea-ui/README.md | Adds DatePicker usage snippet |
| docs/gea-ui/overview.md | Adds Date Picker to interactive components overview table |
| docs/gea-ui/interactive-components.md | Adds DatePicker documentation section + props table |
| examples/showcase/app.tsx | Adds DatePicker demo card and updates footer component count |
| .changeset/add-date-picker.md | Records a minor release changeset for the new component |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export default class DatePicker extends ZagComponent { | ||
| declare open: boolean | ||
| declare value: any[] | ||
| declare valueAsString: string[] | ||
| declare _gridCleanups: Array<() => void> |
There was a problem hiding this comment.
value is declared as any[], which drops useful typing compared to the other Zag-backed components in this package. Consider typing this to the actual date value type(s) used by @zag-js/date-picker (e.g., the DateValue type from @internationalized/date) so consumers get compile-time checking when reading/setting value.
| onValueChange: (details: any) => { | ||
| this.value = details.value | ||
| this.valueAsString = details.valueAsString | ||
| props.onValueChange?.(details) | ||
| }, | ||
| onOpenChange: (details: any) => { |
There was a problem hiding this comment.
onValueChange / onOpenChange callbacks are typed as any, unlike most other components here that use the Zag module’s exported detail types. Please type these details parameters with the corresponding @zag-js/date-picker detail types so downstream code gets accurate IntelliSense and avoids accidental property typos.
| onValueChange: (details: any) => { | |
| this.value = details.value | |
| this.valueAsString = details.valueAsString | |
| props.onValueChange?.(details) | |
| }, | |
| onOpenChange: (details: any) => { | |
| onValueChange: (details: datepicker.ValueChangeDetails) => { | |
| this.value = details.value | |
| this.valueAsString = details.valueAsString | |
| props.onValueChange?.(details) | |
| }, | |
| onOpenChange: (details: datepicker.OpenChangeDetails) => { |
| "@zag-js/clipboard": "^1.37.0", | ||
| "@zag-js/collapsible": "^1.37.0", | ||
| "@zag-js/combobox": "^1.37.0", | ||
| "@zag-js/date-picker": "^1.37.0", | ||
| "@zag-js/dialog": "^1.37.0", |
There was a problem hiding this comment.
Adding @zag-js/date-picker at ^1.37.0 currently pulls in the entire @zag-js/* dependency stack at 1.37.0, while the rest of the repo has resolved to 1.38.0 (per package-lock.json). This version skew increases bundle size and can lead to subtle interop issues; if a 1.38.x date-picker exists, please align versions, otherwise consider pinning all @zag-js/* deps to the same minor or using a workspace-level override/resolution to keep them consistent.
|
@aksuharun, could you please rebase the branch onto main first? It would be easier to review that way. |
Introduces a calendar date picker powered by @zag-js/date-picker with day, month, and year views, keyboard navigation, and full accessibility support. Includes documentation and a regression test covering value binding and view switching. Fixes the showcase footer component count to reflect all 39 exported library components. Resolves: dashersw#8
aksuharun
force-pushed
the
feature/date-picker
branch
from
9c3c0f9 to
60c66fc
Compare
|
Rebased |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/gea-ui/src/components/date-picker.tsx (1)
68-79: Consider adding a clear-trigger element or documenting its absence.The spread map includes
[data-part="clear-trigger"](line 75) but the template doesn't include a corresponding element. While this allows users to optionally add their own clear button, having a built-in clear trigger is a common UX pattern for date pickers.💡 Optional: Add clear-trigger to template
<div data-part="control" class="date-picker-control flex"> <input data-part="input" class="date-picker-input flex h-9 w-full rounded-l-md border border-input bg-transparent px-3 py-1 text-sm shadow-sm focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring" /> + <button + data-part="clear-trigger" + class="date-picker-clear-trigger inline-flex h-9 items-center justify-center border border-l-0 border-input px-2 hover:bg-accent" + aria-label="Clear date" + > + ✕ + </button> <button data-part="trigger" - class="date-picker-trigger inline-flex h-9 items-center justify-center rounded-r-md border border-l-0 border-input px-2 hover:bg-accent" + class="date-picker-trigger inline-flex h-9 items-center justify-center rounded-r-md border border-l-0 border-input px-2 hover:bg-accent" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gea-ui/src/components/date-picker.tsx` around lines 68 - 79, getSpreadMap includes a mapping for '[data-part="clear-trigger"]' pointing to getClearTriggerProps but the component template does not render a corresponding clear button; add a clear-trigger element to the DatePicker template (e.g., a button/span with data-part="clear-trigger") and wire its event handlers and aria attributes using getClearTriggerProps, ensuring it clears the selected date and updates state via the component's existing clear logic (use getClearTriggerProps and any internal clearDate/handleClear methods), or if you prefer not to render a built-in control, remove the '[data-part="clear-trigger"]' entry from getSpreadMap and update documentation to state that consumers must provide their own clear trigger using getClearTriggerProps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/gea-ui/src/components/date-picker.tsx`:
- Around line 262-267: The calendar SVG string CALENDAR_ICON is being inserted
directly into JSX (in the button with data-part="trigger" / class
"date-picker-trigger") which causes it to render as escaped text; mirror the
existing pattern used for CHEVRON_LEFT and CHEVRON_RIGHT by removing
{CALENDAR_ICON} from the JSX and, after the element is mounted (same place you
set prev.innerHTML = CHEVRON_LEFT and next.innerHTML = CHEVRON_RIGHT), find the
trigger element (e.g., via data-part="trigger" or class "date-picker-trigger")
and set triggerElement.innerHTML = CALENDAR_ICON so the SVG is injected
correctly.
---
Nitpick comments:
In `@packages/gea-ui/src/components/date-picker.tsx`:
- Around line 68-79: getSpreadMap includes a mapping for
'[data-part="clear-trigger"]' pointing to getClearTriggerProps but the component
template does not render a corresponding clear button; add a clear-trigger
element to the DatePicker template (e.g., a button/span with
data-part="clear-trigger") and wire its event handlers and aria attributes using
getClearTriggerProps, ensuring it clears the selected date and updates state via
the component's existing clear logic (use getClearTriggerProps and any internal
clearDate/handleClear methods), or if you prefer not to render a built-in
control, remove the '[data-part="clear-trigger"]' entry from getSpreadMap and
update documentation to state that consumers must provide their own clear
trigger using getClearTriggerProps.
🪄 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
Run ID: ccdb1e8e-0ea0-4c50-a2db-e93e17f61cad
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
.changeset/add-date-picker.mddocs/gea-ui/interactive-components.mddocs/gea-ui/overview.mdexamples/showcase/app.tsxpackages/gea-ui/README.mdpackages/gea-ui/package.jsonpackages/gea-ui/src/components/date-picker.tsxpackages/gea-ui/src/index.tspackages/gea-ui/tests/zag-bindings.test.ts
✅ Files skipped from review due to trivial changes (8)
- docs/gea-ui/overview.md
- .changeset/add-date-picker.md
- packages/gea-ui/src/index.ts
- packages/gea-ui/README.md
- packages/gea-ui/package.json
- docs/gea-ui/interactive-components.md
- packages/gea-ui/tests/zag-bindings.test.ts
- examples/showcase/app.tsx
| <button | ||
| data-part="trigger" | ||
| class="date-picker-trigger inline-flex h-9 items-center justify-center rounded-r-md border border-l-0 border-input px-2 hover:bg-accent" | ||
| > | ||
| {CALENDAR_ICON} | ||
| </button> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how other components handle inline SVG/HTML in templates
rg -n "innerHTML|dangerously|<svg" --type=ts --type=tsx -g '*.tsx' packages/gea-ui/src/components/Repository: dashersw/gea
Length of output: 83
🏁 Script executed:
#!/bin/bash
# Search for CALENDAR_ICON definition
rg -n "CALENDAR_ICON" packages/gea-ui/src/Repository: dashersw/gea
Length of output: 596
🏁 Script executed:
#!/bin/bash
# Search for SVG usage and icon handling without problematic file type
rg -n "innerHTML|dangerously|<svg|icon" packages/gea-ui/src/components/ | head -30Repository: dashersw/gea
Length of output: 1691
🏁 Script executed:
#!/bin/bash
# Check the structure of date-picker.tsx to see imports and icon definition
head -50 packages/gea-ui/src/components/date-picker.tsxRepository: dashersw/gea
Length of output: 2467
Fix SVG icon rendering: CALENDAR_ICON should use innerHTML assignment, not direct JSX insertion.
The CALENDAR_ICON string contains SVG markup but is inserted directly in JSX at line 266 as {CALENDAR_ICON}. In this context, it will render as escaped HTML text rather than as an actual SVG element. The same file correctly handles the arrow icons (CHEVRON_LEFT, CHEVRON_RIGHT) using innerHTML assignment at lines 103 and 113. Apply the same pattern to the trigger button:
Change needed
prev.innerHTML = CHEVRON_LEFT
next.innerHTML = CHEVRON_RIGHT
The trigger button should similarly use innerHTML to inject the calendar icon instead of embedding it directly in the JSX template.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/gea-ui/src/components/date-picker.tsx` around lines 262 - 267, The
calendar SVG string CALENDAR_ICON is being inserted directly into JSX (in the
button with data-part="trigger" / class "date-picker-trigger") which causes it
to render as escaped text; mirror the existing pattern used for CHEVRON_LEFT and
CHEVRON_RIGHT by removing {CALENDAR_ICON} from the JSX and, after the element is
mounted (same place you set prev.innerHTML = CHEVRON_LEFT and next.innerHTML =
CHEVRON_RIGHT), find the trigger element (e.g., via data-part="trigger" or class
"date-picker-trigger") and set triggerElement.innerHTML = CALENDAR_ICON so the
SVG is injected correctly.
Resolves #8
UI Components
DatePicker
DatePickercomponent powered by@zag-js/date-pickerspreadPropsbindings when the calendar grid re-renders to avoid leaksDocumentation & Showcase
Documentation
Showcase
DatePickerto the showcase exampleTesting
Zag Bindings
DatePickerRelease Notes
New Features
DatePickercomponent to the UI library.Fixes
Summary by CodeRabbit
New Features
Documentation
Tests