Reason native#1
Conversation
Expose the same JSX-friendly ReactDayPicker API on Melange and native so shared code can render on both targets without separate module names or prop shapes.
Pin server-reason-react to the git source and update the native date construction/docs so this package still builds against that runtime API.
📝 WalkthroughWalkthroughIntroduces a platform-aware public ReactDayPicker component that delegates to JS (ClientImpl) or native (ServerImpl) implementations, adds a full native day-picker renderer, expands examples, parity/fuzz tests, Playwright browser tests, and updates build/config (dune, opam) and documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Wrapper as ReactDayPicker<br/>(public make)
participant Platform as Runtime.platform
participant Client as ClientImpl<br/>(JS)
participant JSLib as react-day-picker<br/>(external)
participant Server as ServerImpl<br/>(native)
participant Native as ReactDayPickerNative<br/>(native impl)
App->>Wrapper: render(~mode, ~selected, ~onSelect, ...)
Wrapper->>Platform: switch%platform
alt JS
Platform->>Client: delegate
Client->>Client: convert variants -> raw JS (toJsRangeDate / Obj.magic)
Client->>JSLib: React.createElement(make, props)
JSLib-->>Client: element
Client-->>Wrapper: element
else Native
Platform->>Server: delegate
Server->>Server: convert variants -> native types (toNativeRangeDate)
Server->>Native: call ReactDayPickerNative.make with converted props
Native-->>Server: element
Server-->>Wrapper: element
end
Wrapper-->>App: rendered component
Estimated Code Review Effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (12)
reason-react-day-picker.opam.template (1)
1-3: Consider pinning to a specific commit or tag for reproducibility.Pinning
server-reason-react.devto the repository HEAD without a specific commit hash or tag means builds may produce different results over time as the upstream repo changes. If this is intentional during active development, consider documenting that this should be pinned to a release tag before publishing a stable version.♻️ Example with commit pinning
pin-depends: [ - [ "server-reason-react.dev" "git+https://github.com/ml-in-barcelona/server-reason-react.git" ] + [ "server-reason-react.dev" "git+https://github.com/ml-in-barcelona/server-reason-react.git#<commit-sha>" ] ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@reason-react-day-picker.opam.template` around lines 1 - 3, The pin-depends entry currently references "server-reason-react.dev" at repository HEAD; change the pin-depends value to reference a specific commit hash or release tag (e.g., replace the git+https URL with git+https://...#<commit-hash-or-tag>) to ensure reproducible builds, and if leaving it unpinned during active development is intentional, add a short comment in the template noting it must be pinned to a release tag before publishing; update the pin-depends array and the "server-reason-react.dev" entry accordingly.ReactDayPickerNative.re (1)
332-337: Week number calculation may differ from ISO 8601.The
getWeekNumberimplementation uses a simplified algorithm based on the first day of the year. ISO 8601 week numbering has specific rules (week 1 contains the first Thursday). If strict ISO compliance is needed, this may require adjustment. For most calendar UIs, this approximation is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ReactDayPickerNative.re` around lines 332 - 337, The getWeekNumber function currently computes weeks by dividing days since year start using dateYear, newDate, dayOfWeek and getDayOfYear, which does not follow ISO 8601 (week 1 contains the year's first Thursday); to fix, replace or augment getWeekNumber to implement ISO rules: determine the Thursday-based week anchor (adjust the date by (4 - dayOfWeek) days), compute the year of that anchor, then compute week number as 1 + floor((dayOfYear(anchor) - 1) / 7); update getWeekNumber to use this algorithm (keeping helpers dateYear, newDate, dayOfWeek, getDayOfYear) so week numbers match ISO 8601 when required.ReactDayPicker.re (1)
93-106:Obj.magicusage is appropriate for JS interop but inherently unsafe.The use of
Obj.magicto convert polymorphic variants to the raw values expected by react-day-picker is a common pattern in Melange FFI. However, this bypasses type checking:
- If react-day-picker's API changes, these conversions will silently produce incorrect values
- Consider adding inline comments documenting the expected JS shapes
This is acceptable for library bindings where the alternative would be much more verbose.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ReactDayPicker.re` around lines 93 - 106, The Obj.magic conversions in toJsSelected and toJsOnSelect (and the use of toJsRangeDate/fromJsRangeDate) are unsafe for JS interop; add concise inline comments above toJsSelected and toJsOnSelect documenting the exact JS shapes and callback signatures expected by react-day-picker for `Single`, `Multiple`, and `Range` (e.g., Single -> Date, Multiple -> Array<Date>, Range -> { from?: Date, to?: Date }, and the Range callback receives that shape), and note that Obj.magic is used intentionally for brevity so future API changes are easier to spot; keep the comments next to the function definitions for quick discovery.example/shared/ExampleDayPickers.re (1)
3-12: UnusedmakeDatehelper function.This helper function is defined but never called in this file. The date construction on lines 14-24 uses inline
Js.Date.makecalls instead.🧹 Either remove or use the helper
Option 1 - Remove unused code:
-let makeDate = (~year: int, ~month: int, ~day: int): Js.Date.t => - Js.Date.make( - ~year=float_of_int(year), - ~month=float_of_int(month), - ~date=float_of_int(day), - ~hours=12.0, - ~minutes=0.0, - ~seconds=0.0, - (), - ); -Option 2 - Use the helper for
closeDate:-let closeDate = Js.Date.make( - ~year=Js.Date.getFullYear(today), - ~month=Js.Date.getMonth(today), - ~date=Js.Date.getDate(today) +. 4.0, - ~hours=12.0, - ~minutes=0.0, - ~seconds=0.0, - (), -); +let closeDate = makeDate( + ~year=int_of_float(Js.Date.getFullYear(today)), + ~month=int_of_float(Js.Date.getMonth(today)), + ~day=int_of_float(Js.Date.getDate(today)) + 4, +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/shared/ExampleDayPickers.re` around lines 3 - 12, The function makeDate is defined but unused; either remove it or replace the inline Js.Date.make call used for closeDate with the helper. Locate makeDate and the closeDate construction in ExampleDayPickers.re, then either delete the makeDate declaration or refactor closeDate to call makeDate(~year, ~month, ~day) (keep the existing hours/minutes/seconds behavior by ensuring makeDate produces noon) so there’s no unused helper left behind.example/fuzz/FuzzRenderer.re (2)
54-68: Partial range handling differs from RangeRenderer.re.This logic returns
Js.Undefined.fromOption(None)when onlyfromis present (withoutto), or when onlytowould be present (not handled). In contrast,RangeRenderer.re(lines 12-24) constructs a valid range object with one bound set and the other undefined.If partial range selection should be testable via fuzz, consider aligning the logic:
♻️ Handle partial ranges consistently
| "range" => let range = switch (config.rangeFrom) { | Some(from) => switch (config.rangeTo) { | Some(to_) => Js.Undefined.return({ ReactDayPicker.from: Js.Undefined.return(from), ReactDayPicker.to_: Js.Undefined.return(to_), - }); - | None => Js.Undefined.fromOption(None) + }) + | None => + Js.Undefined.return({ + ReactDayPicker.from: Js.Undefined.return(from), + ReactDayPicker.to_: Js.Undefined.fromOption(None), + }) }; - | None => Js.Undefined.fromOption(None) + | None => + switch (config.rangeTo) { + | Some(to_) => + Js.Undefined.return({ + ReactDayPicker.from: Js.Undefined.fromOption(None), + ReactDayPicker.to_: Js.Undefined.return(to_), + }) + | None => Js.Undefined.fromOption(None) + } }; `Range(range);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/fuzz/FuzzRenderer.re` around lines 54 - 68, The "range" branch in FuzzRenderer.re currently returns Js.Undefined.fromOption(None) when only one bound is present, which prevents creating partial ranges; update the logic to mirror RangeRenderer.re by constructing a range object even when only from or only to is present: when config.rangeFrom is Some(from) and config.rangeTo is None, return a Range where ReactDayPicker.from is Js.Undefined.return(from) and ReactDayPicker.to_ is Js.Undefined.fromOption(None); likewise when rangeTo is Some(to_) and rangeFrom is None construct a Range with ReactDayPicker.to_ set and ReactDayPicker.from as Js.Undefined.fromOption(None); keep the existing full-range case unchanged so partial selections are testable by the fuzzer.
19-31: Redundant identity transformations can be simplified.The switch statements for
captionLayoutandnavLayoutare identity transformations that map each variant to itself. Sinceconfig.captionLayoutis already of typeoption(Scenario.captionLayout), this can be simplified.♻️ Suggested simplification
- let captionLayout = switch (config.captionLayout) { - | Some(`Label) => Some(`Label) - | Some(`Dropdown) => Some(`Dropdown) - | Some(`DropdownMonths) => Some(`DropdownMonths) - | Some(`DropdownYears) => Some(`DropdownYears) - | None => None - }; - let navLayout = switch (config.navLayout) { - | Some(`Around) => Some(`Around) - | Some(`After) => Some(`After) - | None => None - }; + let captionLayout = config.captionLayout; + let navLayout = config.navLayout;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/fuzz/FuzzRenderer.re` around lines 19 - 31, The switch blocks in renderFuzzConfig (for captionLayout and navLayout) are identity mappings; replace the switch logic by directly using the existing option values: assign captionLayout = config.captionLayout and navLayout = config.navLayout (i.e., remove the redundant switch over config.captionLayout and config.navLayout and use the fields as-is) so the function is simpler and equivalent.example/shared/RangeScenario.re (3)
8-17: Consider extractingmakeDateto a shared utility module.This helper is duplicated verbatim in
FuzzTestRunner.re(lines 25-34). Extracting it to a shared module (e.g.,DateUtils.re) would improve maintainability and reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/shared/RangeScenario.re` around lines 8 - 17, The helper function makeDate is duplicated and should be extracted into a shared utility module; create a new module (e.g., DateUtils.re) that exports makeDate with the same signature (let makeDate = (~year:int, ~month:int, ~day:int): Js.Date.t => ...), replace the inline implementations in RangeScenario.re and FuzzTestRunner.re with imports/uses of DateUtils.makeDate, and update any module references or build entries so both modules use the single shared implementation.
19-21: Unused variablenextWeek.The
nextWeekconstant is defined but never used in this module. Consider removing it to avoid dead code, or use it in the scenario definitions if there's a "range-week-long" scenario planned.🧹 Proposed removal
let today = makeDate(~year=2026, ~month=2, ~day=21); let tomorrow = makeDate(~year=2026, ~month=2, ~day=22); -let nextWeek = makeDate(~year=2026, ~month=2, ~day=28);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/shared/RangeScenario.re` around lines 19 - 21, The constant `nextWeek` is declared via `makeDate(~year=2026, ~month=2, ~day=28)` but never used; either remove the `nextWeek` declaration to eliminate dead code or integrate `nextWeek` into your scenario definitions (e.g., a week-long range scenario) where `today` and `tomorrow` are used; update any scenario functions that should reference a week range to use `nextWeek` (or delete the `nextWeek` binding if no such scenario is needed).
30-40: Fallback case inbyNamesilently uses default values.The wildcard pattern at line 37 returns
(Some(today), Some(today))for unrecognized names without warning. Sincecurrent()already validates againstall, this is fine for internal use, but the function is public. Consider documenting this behavior or making the fallback explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/shared/RangeScenario.re` around lines 30 - 40, The public function byName currently silently maps unknown names to (Some(today), Some(tomorrow)) via the wildcard branch; update byName to make this behavior explicit by either (A) replacing the wildcard branch with an explicit failure (e.g., raise/invalid argument including the unknown name) so callers get immediate feedback, or (B) keep the fallback but add a clear docstring/comment on byName describing that unknown names will map to (Some(today), Some(tomorrow)) and that current()/all validation is expected elsewhere; reference the byName function and the today/tomorrow identifiers when making the change.example/fuzz/FuzzTestRunner.re (1)
25-34: DuplicatemakeDatehelper.This function is identical to
RangeScenario.makeDate. Consider extracting to a shared utility module to avoid duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/fuzz/FuzzTestRunner.re` around lines 25 - 34, The makeDate implementation in FuzzTestRunner.re duplicates RangeScenario.makeDate; extract the shared implementation into a common utility (e.g., DateHelpers.makeDate) and replace the local makeDate with an import/use of that shared function; update FuzzTestRunner.re to call DateHelpers.makeDate instead of its own makeDate and remove the duplicate definition, and update any other modules using the duplicated function to reference the single shared symbol (e.g., DateHelpers.makeDate) so only one implementation remains.example/shared/Scenario.re (1)
84-97: Consider extracting the CLI argument parsing pattern.This
current()function follows the same pattern asRangeScenario.current(): read last CLI arg, validate against an allow-list, fallback to default. Consider extracting a shared helper like:let currentScenarioName = (~all, ~default) => ...This would reduce duplication and ensure consistent CLI handling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/shared/Scenario.re` around lines 84 - 97, The current() function duplicates the CLI-arg parsing found in RangeScenario.current(); extract that logic into a shared helper (e.g., currentScenarioName with signature like (~all, ~default) => string) then have current() and RangeScenario.current() call this helper and pass their allow-list and default; locate and replace the duplicated conditional that reads Sys.argv, picks the last arg, validates with Array.exists against all, and returns either the candidate or "default", and ensure you continue to pass the resulting name into byName(name) in current() and the equivalent call in RangeScenario.current().example/fuzz/FuzzGenerator.re (1)
133-148: Date rollover may produce unexpected month boundary crossings.When
fromDayis late in the month (e.g., day 28), adding up to 14 days (toDay = fromDay +. 14.0= 42) will causeJs.Dateto roll over into the next month. This may be acceptable for fuzz testing, but could produce ranges that span months unexpectedly.If you want to constrain
rangeToto the same month, clamptoDayto 28 (or calculate based on actual month length).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/fuzz/FuzzGenerator.re` around lines 133 - 148, The current rangeTo calculation (involving rangeTo, rangeFrom, fromDay, toDay, randomInt, Js.Date.make) can roll into the next month; to fix, compute the last day of the month for Js.Date.getMonth(from)/Js.Date.getFullYear(from) (e.g., by constructing the first day of the next month and subtracting one day or using the standard JS date trick) and clamp toDay to Math.min(toDay, lastDayOfMonth) before calling Js.Date.make so the generated range stays within the same month (alternatively, clamp to 28 if you intentionally want a fixed safe cap).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@example/fuzz/FuzzGenerator.re`:
- Around line 174-181: generateConfig currently uses Random.int(3) before
seeding the PRNG, making mode selection non-deterministic; seed the PRNG by
calling Random.init(seed) at the start of generateConfig (before Random.int(3))
so the selection is derived from the provided seed, and ensure the downstream
generators (generateSingleConfig, generateMultipleConfig, generateRangeConfig)
either rely on this seeded state or do not re-init the RNG in a conflicting way.
In `@example/fuzz/FuzzTestRunner.re`:
- Around line 19-23: The testFuzzConfig helper uses a tautological assertion
(Alcotest.check(Alcotest.bool) ... true, true) which never validates the
runNativeTest result; change testFuzzConfig to capture the value returned by
runNativeTest and assert on its contents instead (for example, call let output =
runNativeTest(config) and then use Alcotest.check(Alcotest.string) or
Alcotest.(check (list string)) to verify expected HTML substrings, DOM markers,
or compare against a stored snapshot); update references in testFuzzConfig and
any callers so assertions verify meaningful properties of output rather than
always-true literals.
In `@example/README.md`:
- Line 13: Update the skipped heading levels by changing the "### Run the native
renderer" heading to an h2 (## Run the native renderer) and likewise convert the
other top-level section headings currently using "###" (the headings at the
other section titles) to "##" so the document flows from the existing h1 to h2
before any h3s; ensure subsequent subheadings that are intended to be nested
remain at h3.
---
Nitpick comments:
In `@example/fuzz/FuzzGenerator.re`:
- Around line 133-148: The current rangeTo calculation (involving rangeTo,
rangeFrom, fromDay, toDay, randomInt, Js.Date.make) can roll into the next
month; to fix, compute the last day of the month for
Js.Date.getMonth(from)/Js.Date.getFullYear(from) (e.g., by constructing the
first day of the next month and subtracting one day or using the standard JS
date trick) and clamp toDay to Math.min(toDay, lastDayOfMonth) before calling
Js.Date.make so the generated range stays within the same month (alternatively,
clamp to 28 if you intentionally want a fixed safe cap).
In `@example/fuzz/FuzzRenderer.re`:
- Around line 54-68: The "range" branch in FuzzRenderer.re currently returns
Js.Undefined.fromOption(None) when only one bound is present, which prevents
creating partial ranges; update the logic to mirror RangeRenderer.re by
constructing a range object even when only from or only to is present: when
config.rangeFrom is Some(from) and config.rangeTo is None, return a Range where
ReactDayPicker.from is Js.Undefined.return(from) and ReactDayPicker.to_ is
Js.Undefined.fromOption(None); likewise when rangeTo is Some(to_) and rangeFrom
is None construct a Range with ReactDayPicker.to_ set and ReactDayPicker.from as
Js.Undefined.fromOption(None); keep the existing full-range case unchanged so
partial selections are testable by the fuzzer.
- Around line 19-31: The switch blocks in renderFuzzConfig (for captionLayout
and navLayout) are identity mappings; replace the switch logic by directly using
the existing option values: assign captionLayout = config.captionLayout and
navLayout = config.navLayout (i.e., remove the redundant switch over
config.captionLayout and config.navLayout and use the fields as-is) so the
function is simpler and equivalent.
In `@example/fuzz/FuzzTestRunner.re`:
- Around line 25-34: The makeDate implementation in FuzzTestRunner.re duplicates
RangeScenario.makeDate; extract the shared implementation into a common utility
(e.g., DateHelpers.makeDate) and replace the local makeDate with an import/use
of that shared function; update FuzzTestRunner.re to call DateHelpers.makeDate
instead of its own makeDate and remove the duplicate definition, and update any
other modules using the duplicated function to reference the single shared
symbol (e.g., DateHelpers.makeDate) so only one implementation remains.
In `@example/shared/ExampleDayPickers.re`:
- Around line 3-12: The function makeDate is defined but unused; either remove
it or replace the inline Js.Date.make call used for closeDate with the helper.
Locate makeDate and the closeDate construction in ExampleDayPickers.re, then
either delete the makeDate declaration or refactor closeDate to call
makeDate(~year, ~month, ~day) (keep the existing hours/minutes/seconds behavior
by ensuring makeDate produces noon) so there’s no unused helper left behind.
In `@example/shared/RangeScenario.re`:
- Around line 8-17: The helper function makeDate is duplicated and should be
extracted into a shared utility module; create a new module (e.g., DateUtils.re)
that exports makeDate with the same signature (let makeDate = (~year:int,
~month:int, ~day:int): Js.Date.t => ...), replace the inline implementations in
RangeScenario.re and FuzzTestRunner.re with imports/uses of DateUtils.makeDate,
and update any module references or build entries so both modules use the single
shared implementation.
- Around line 19-21: The constant `nextWeek` is declared via
`makeDate(~year=2026, ~month=2, ~day=28)` but never used; either remove the
`nextWeek` declaration to eliminate dead code or integrate `nextWeek` into your
scenario definitions (e.g., a week-long range scenario) where `today` and
`tomorrow` are used; update any scenario functions that should reference a week
range to use `nextWeek` (or delete the `nextWeek` binding if no such scenario is
needed).
- Around line 30-40: The public function byName currently silently maps unknown
names to (Some(today), Some(tomorrow)) via the wildcard branch; update byName to
make this behavior explicit by either (A) replacing the wildcard branch with an
explicit failure (e.g., raise/invalid argument including the unknown name) so
callers get immediate feedback, or (B) keep the fallback but add a clear
docstring/comment on byName describing that unknown names will map to
(Some(today), Some(tomorrow)) and that current()/all validation is expected
elsewhere; reference the byName function and the today/tomorrow identifiers when
making the change.
In `@example/shared/Scenario.re`:
- Around line 84-97: The current() function duplicates the CLI-arg parsing found
in RangeScenario.current(); extract that logic into a shared helper (e.g.,
currentScenarioName with signature like (~all, ~default) => string) then have
current() and RangeScenario.current() call this helper and pass their allow-list
and default; locate and replace the duplicated conditional that reads Sys.argv,
picks the last arg, validates with Array.exists against all, and returns either
the candidate or "default", and ensure you continue to pass the resulting name
into byName(name) in current() and the equivalent call in
RangeScenario.current().
In `@ReactDayPicker.re`:
- Around line 93-106: The Obj.magic conversions in toJsSelected and toJsOnSelect
(and the use of toJsRangeDate/fromJsRangeDate) are unsafe for JS interop; add
concise inline comments above toJsSelected and toJsOnSelect documenting the
exact JS shapes and callback signatures expected by react-day-picker for
`Single`, `Multiple`, and `Range` (e.g., Single -> Date, Multiple ->
Array<Date>, Range -> { from?: Date, to?: Date }, and the Range callback
receives that shape), and note that Obj.magic is used intentionally for brevity
so future API changes are easier to spot; keep the comments next to the function
definitions for quick discovery.
In `@ReactDayPickerNative.re`:
- Around line 332-337: The getWeekNumber function currently computes weeks by
dividing days since year start using dateYear, newDate, dayOfWeek and
getDayOfYear, which does not follow ISO 8601 (week 1 contains the year's first
Thursday); to fix, replace or augment getWeekNumber to implement ISO rules:
determine the Thursday-based week anchor (adjust the date by (4 - dayOfWeek)
days), compute the year of that anchor, then compute week number as 1 +
floor((dayOfYear(anchor) - 1) / 7); update getWeekNumber to use this algorithm
(keeping helpers dateYear, newDate, dayOfWeek, getDayOfYear) so week numbers
match ISO 8601 when required.
In `@reason-react-day-picker.opam.template`:
- Around line 1-3: The pin-depends entry currently references
"server-reason-react.dev" at repository HEAD; change the pin-depends value to
reference a specific commit hash or release tag (e.g., replace the git+https URL
with git+https://...#<commit-hash-or-tag>) to ensure reproducible builds, and if
leaving it unpinned during active development is intentional, add a short
comment in the template noting it must be pinned to a release tag before
publishing; update the pin-depends array and the "server-reason-react.dev" entry
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 164c80de-768f-4775-9bb3-3451de12778c
📒 Files selected for processing (28)
.gitignoreREADME.mdReactDayPicker.reReactDayPickerNative.redunedune-projectexample/README.mdexample/check-parity.shexample/check-range-parity.shexample/fuzz-test.shexample/fuzz/FuzzGenerator.reexample/fuzz/FuzzRenderer.reexample/fuzz/FuzzTestRunner.reexample/fuzz/duneexample/js/JsRenderer.reexample/js/RangeJsRenderer.reexample/js/duneexample/native/NativeRenderer.reexample/native/RangeNativeRenderer.reexample/native/duneexample/shared/ExampleDayPickers.reexample/shared/RangeRenderer.reexample/shared/RangeScenario.reexample/shared/Scenario.reexample/shared/SharedFixture.renative/dunereason-react-day-picker.opamreason-react-day-picker.opam.template
| single picker and a range picker for the same fixture values in | ||
| `example/shared/SharedFixture.re`, including `footer` via `React.string(...)`. | ||
|
|
||
| ### Run the native renderer |
There was a problem hiding this comment.
Heading levels skip from h1 to h3.
The document jumps directly from # (h1) to ### (h3) without an intermediate ## (h2). This breaks the document outline hierarchy and can affect screen reader navigation.
Consider using ## for these section headings, or adding an intermediate h2 section header.
📝 Suggested fix
-### Run the native renderer
+## Run the native rendererApply similar changes to other ### headings (lines 19, 30, 42, 49).
📝 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.
| ### Run the native renderer | |
| ## Run the native renderer |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 13-13: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@example/README.md` at line 13, Update the skipped heading levels by changing
the "### Run the native renderer" heading to an h2 (## Run the native renderer)
and likewise convert the other top-level section headings currently using "###"
(the headings at the other section titles) to "##" so the document flows from
the existing h1 to h2 before any h3s; ensure subsequent subheadings that are
intended to be nested remain at h3.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
example/fuzz/FuzzTestRunner.re (1)
64-210:⚠️ Potential issue | 🟠 MajorRendered output is still unverified across suites.
runNativeTestreturns normalized HTML, but every suite ignores it. These tests currently only check that rendering does not throw, so regressions in markup can pass silently.✅ Suggested minimal fix (assert non-empty output, then reuse)
+let assertRender = config => { + let output = runNativeTest(config); + Alcotest.(check bool)( + "render output should be non-empty", + true, + String.length(output) > 0, + ); +}; + ... - ignore(runNativeTest(config)); + assertRender(config); ... - ignore(runNativeTest(config)); + assertRender(config);Apply the replacement to all current
ignore(runNativeTest(config))call sites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/fuzz/FuzzTestRunner.re` around lines 64 - 210, The tests call runNativeTest(config) and ignore its normalized-HTML result, so markup regressions go unnoticed; change each ignore(runNativeTest(config)) in the Alcobar.test_case blocks to capture the returned string (from runNativeTest), assert it is non-empty (e.g., length > 0 or not equal to ""), and then continue to discard or reuse that variable as needed—update all occurrences in the fuzz suites (e.g., the "range-*" cases, singleTests, multipleTests, rangeRandomTests) so each test verifies the returned HTML is not empty instead of simply ignoring it.
🧹 Nitpick comments (3)
example/fuzz/FuzzTestRunner.re (1)
42-179: Range scenario configs are heavily duplicated.The five explicit range test cases repeat nearly identical record construction, which increases maintenance risk when fields evolve. Consider extracting a small builder/helper for shared defaults plus
{rangeFrom, rangeTo, name}overrides.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/fuzz/FuzzTestRunner.re` around lines 42 - 179, The test configs duplicate the same record for range scenarios; create a helper (e.g., makeRangeConfig or buildFuzzConfig) that returns the common base record used in these tests and accepts overrides for FuzzGenerator.name, rangeFrom and rangeTo (and any other per-test fields), then replace each inline config in the "range-same-day", "range-multi-day", "range-start-only", "range-end-only", and "range-week-long" Alcobar.test_case blocks with calls to that helper and pass the appropriate name/range values before calling runNativeTest(config).example/check-range-parity.sh (1)
12-37: Reduce drift by extracting shared parity harness logic.This script and
example/check-parity.shduplicate most control flow (setup, tempdir, build/test, loop, diff). Pulling shared logic into a common helper script will reduce maintenance drift as scenarios and commands evolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/check-range-parity.sh` around lines 12 - 37, Extract the duplicated setup/teardown and loop+diff logic into a shared helper script and call it from this file: move tmpdir="$(mktemp -d)", trap 'rm -rf "$tmpdir"' EXIT, the build/test commands (dune build ... and dune exec ./example/native/RangeNativeTest.exe), and the for-loop that runs scenarios/diffs into a new helper (e.g. parity_harness.sh with functions like setup_tmpdir, build_and_test, run_parity <renderer-js> <renderer-native> <scenarios>); then replace the duplicated blocks in example/check-range-parity.sh by invoking that helper and passing the specific renderer paths ("_build/default/example/js/render/example/js/RangeJsRenderer.re.js" and "./example/native/RangeNativeRenderer.exe") and the scenarios array, ensuring the helper uses the same exit behavior and prints the same failure/success messages.example/native/test/NativeTest.re (1)
1-40: Extract shared test helpers used by both native test files.
contains, process execution, output capture, and exit-status handling are duplicated here and inexample/native/test/RangeNativeTest.re. A shared helper module would reduce drift and make future assertion updates one-place changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/native/test/NativeTest.re` around lines 1 - 40, Extract the duplicated helpers (contains, the process-run/capture logic around Unix.open_process_in and readAll, and the exit-status handling) into a single shared module (e.g., NativeTestHelpers) and reuse it from both example/native/test/NativeTest.re and RangeNativeTest.re: move the contains function and the run-and-capture routine (including status switch that turns non-zero/ signal/stop into Alcotest.fail) into exported functions like contains: (string,string)=>bool and runProcessCapture: string => (output:string) or runProcessAssertOk: string => string; then replace the inline implementations in testScenario (and the other test file) with calls to these helpers so both tests import NativeTestHelpers and call the shared functions instead of duplicating code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@example/native/test/RangeNativeTest.re`:
- Around line 38-39: Add an assertion that the renderer output contains the end
marker "RENDER_END" so the test verifies the full start-to-end contract; in the
same test where Alcotest.check(Alcotest.bool) is used with contains(output^,
"RENDER_START") and contains(output^, "rdp-root"), add a similar
Alcotest.check(Alcotest.bool) that asserts contains(output^, "RENDER_END") (use
the same pattern and message style) to ensure output is not truncated.
In `@example/test-browser/DayPickerBrowserTest.re`:
- Around line 48-69: The test currently drops the TestServer.stop promise and
skips cleanup on failure; update the run() promise chain and outer catch to
always await and perform cleanup: ensure the inner finally/then branch awaits
TestServer.stop(server) (don't just ignore it) and awaits
browser->Playwright.close before calling resolve, and modify the outer catch
block to first attempt to close the browser (Playwright.close) and await
TestServer.stop(server) before calling BrowserTestUtils.exitProcess(1) and
resolve; reference the existing symbols TestServer.stop, Playwright.close,
BrowserTestUtils.exitProcess, run and resolve when making these changes so both
success and failure paths always clean up and wait for server shutdown.
In `@example/test-browser/DayPickerExample.re`:
- Around line 41-77: The component uses refs (openDateRef, closeDateRef) tied to
module-level openDate/closeDate so the selected prop never changes and the
picker is effectively read-only; replace the refs with React state (useState)
inside rangeDayPicker — create [openDate, setOpenDate] and [closeDate,
setCloseDate] via useState, pass those state values into the selected prop
instead of the module constants, and update them in the onSelect handler (use
the setOpenDate/setCloseDate state setters) so re-renders occur when the user
selects a range; remove openDateRef/closeDateRef and their update helpers once
state is used.
In `@example/test-browser/index.html`:
- Line 1: The file index.html currently starts with an invalid doctype token
"<!>"; replace this with a proper HTML5 doctype declaration by updating the top
of the file to use "<!DOCTYPE html>" so browsers render in standards mode;
ensure this change is applied to the very first line in
example/test-browser/index.html before any other content.
---
Duplicate comments:
In `@example/fuzz/FuzzTestRunner.re`:
- Around line 64-210: The tests call runNativeTest(config) and ignore its
normalized-HTML result, so markup regressions go unnoticed; change each
ignore(runNativeTest(config)) in the Alcobar.test_case blocks to capture the
returned string (from runNativeTest), assert it is non-empty (e.g., length > 0
or not equal to ""), and then continue to discard or reuse that variable as
needed—update all occurrences in the fuzz suites (e.g., the "range-*" cases,
singleTests, multipleTests, rangeRandomTests) so each test verifies the returned
HTML is not empty instead of simply ignoring it.
---
Nitpick comments:
In `@example/check-range-parity.sh`:
- Around line 12-37: Extract the duplicated setup/teardown and loop+diff logic
into a shared helper script and call it from this file: move tmpdir="$(mktemp
-d)", trap 'rm -rf "$tmpdir"' EXIT, the build/test commands (dune build ... and
dune exec ./example/native/RangeNativeTest.exe), and the for-loop that runs
scenarios/diffs into a new helper (e.g. parity_harness.sh with functions like
setup_tmpdir, build_and_test, run_parity <renderer-js> <renderer-native>
<scenarios>); then replace the duplicated blocks in
example/check-range-parity.sh by invoking that helper and passing the specific
renderer paths
("_build/default/example/js/render/example/js/RangeJsRenderer.re.js" and
"./example/native/RangeNativeRenderer.exe") and the scenarios array, ensuring
the helper uses the same exit behavior and prints the same failure/success
messages.
In `@example/fuzz/FuzzTestRunner.re`:
- Around line 42-179: The test configs duplicate the same record for range
scenarios; create a helper (e.g., makeRangeConfig or buildFuzzConfig) that
returns the common base record used in these tests and accepts overrides for
FuzzGenerator.name, rangeFrom and rangeTo (and any other per-test fields), then
replace each inline config in the "range-same-day", "range-multi-day",
"range-start-only", "range-end-only", and "range-week-long" Alcobar.test_case
blocks with calls to that helper and pass the appropriate name/range values
before calling runNativeTest(config).
In `@example/native/test/NativeTest.re`:
- Around line 1-40: Extract the duplicated helpers (contains, the
process-run/capture logic around Unix.open_process_in and readAll, and the
exit-status handling) into a single shared module (e.g., NativeTestHelpers) and
reuse it from both example/native/test/NativeTest.re and RangeNativeTest.re:
move the contains function and the run-and-capture routine (including status
switch that turns non-zero/ signal/stop into Alcotest.fail) into exported
functions like contains: (string,string)=>bool and runProcessCapture: string =>
(output:string) or runProcessAssertOk: string => string; then replace the inline
implementations in testScenario (and the other test file) with calls to these
helpers so both tests import NativeTestHelpers and call the shared functions
instead of duplicating code.
🪄 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: 5cd865e6-644c-462e-a3d0-97d58c2f9cf7
⛔ Files ignored due to path filters (5)
example/test-browser/dist/example/test-browser/generated/BrowserTestUtils.mjsis excluded by!**/dist/**,!**/generated/**example/test-browser/dist/example/test-browser/generated/DayPickerBrowserTest.mjsis excluded by!**/dist/**,!**/generated/**example/test-browser/dist/example/test-browser/generated/DayPickerExample.mjsis excluded by!**/dist/**,!**/generated/**example/test-browser/dist/example/test-browser/generated/Playwright.mjsis excluded by!**/dist/**,!**/generated/**example/test-browser/dist/example/test-browser/generated/TestServer.mjsis excluded by!**/dist/**,!**/generated/**
📒 Files selected for processing (21)
.gitignoredune-projectexample/check-parity.shexample/check-range-parity.shexample/fuzz-test.shexample/fuzz/FuzzGenerator.reexample/fuzz/FuzzTestRunner.reexample/fuzz/duneexample/native/test/NativeTest.reexample/native/test/RangeNativeTest.reexample/native/test/duneexample/test-browser/BrowserTestUtils.reexample/test-browser/DayPickerBrowserTest.reexample/test-browser/DayPickerExample.reexample/test-browser/Playwright.reexample/test-browser/TestServer.reexample/test-browser/duneexample/test-browser/index.htmlexample/test-browser/run-browser-test.shpackage.jsonreason-react-day-picker.opam
✅ Files skipped from review due to trivial changes (4)
- .gitignore
- reason-react-day-picker.opam
- example/test-browser/dune
- package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- example/fuzz/dune
- dune-project
| Alcotest.check(Alcotest.bool)("Output contains RENDER_START", true, contains(output^, "RENDER_START")); | ||
| Alcotest.check(Alcotest.bool)("Output contains rdp-root", true, contains(output^, "rdp-root")); |
There was a problem hiding this comment.
Assert RENDER_END to fully validate renderer output contract.
Current checks can pass even if output is truncated after start/content. Add an explicit end-marker assertion.
Proposed patch
Alcotest.check(Alcotest.bool)("Output contains RENDER_START", true, contains(output^, "RENDER_START"));
+ Alcotest.check(Alcotest.bool)("Output contains RENDER_END", true, contains(output^, "RENDER_END"));
Alcotest.check(Alcotest.bool)("Output contains rdp-root", true, contains(output^, "rdp-root"));📝 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.
| Alcotest.check(Alcotest.bool)("Output contains RENDER_START", true, contains(output^, "RENDER_START")); | |
| Alcotest.check(Alcotest.bool)("Output contains rdp-root", true, contains(output^, "rdp-root")); | |
| Alcotest.check(Alcotest.bool)("Output contains RENDER_START", true, contains(output^, "RENDER_START")); | |
| Alcotest.check(Alcotest.bool)("Output contains RENDER_END", true, contains(output^, "RENDER_END")); | |
| Alcotest.check(Alcotest.bool)("Output contains rdp-root", true, contains(output^, "rdp-root")); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@example/native/test/RangeNativeTest.re` around lines 38 - 39, Add an
assertion that the renderer output contains the end marker "RENDER_END" so the
test verifies the full start-to-end contract; in the same test where
Alcotest.check(Alcotest.bool) is used with contains(output^, "RENDER_START") and
contains(output^, "rdp-root"), add a similar Alcotest.check(Alcotest.bool) that
asserts contains(output^, "RENDER_END") (use the same pattern and message style)
to ensure output is not truncated.
| |> then_(_ => browser->Playwright.close) | ||
| |> then_(_ => { | ||
| ignore(TestServer.stop(server)); | ||
| resolve(); | ||
| }) | ||
| ) | ||
| ) | ||
| ); | ||
| }; | ||
|
|
||
| let () = | ||
| run() | ||
| |> then_(_ => { | ||
| Js.log("DayPicker browser tests passed!"); | ||
| BrowserTestUtils.exitProcess(0); | ||
| resolve(); | ||
| }) | ||
| |> catch(error => { | ||
| Js.log2("DayPicker browser tests failed:", error); | ||
| BrowserTestUtils.exitProcess(1); | ||
| resolve(); | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -type f -name "*DayPickerBrowserTest*"Repository: SoftwareDeployed/reason-react-day-picker
Length of output: 254
🏁 Script executed:
git ls-files | grep -i "dippicker.*browser"Repository: SoftwareDeployed/reason-react-day-picker
Length of output: 66
🏁 Script executed:
git ls-files | grep example/test-browserRepository: SoftwareDeployed/reason-react-day-picker
Length of output: 761
🏁 Script executed:
wc -l example/test-browser/DayPickerBrowserTest.reRepository: SoftwareDeployed/reason-react-day-picker
Length of output: 131
🏁 Script executed:
cat -n example/test-browser/DayPickerBrowserTest.reRepository: SoftwareDeployed/reason-react-day-picker
Length of output: 3031
🏁 Script executed:
cat -n example/test-browser/TestServer.reRepository: SoftwareDeployed/reason-react-day-picker
Length of output: 2263
🏁 Script executed:
cat -n example/test-browser/BrowserTestUtils.reRepository: SoftwareDeployed/reason-react-day-picker
Length of output: 1188
Ensure cleanup runs on failure too, and await server stop.
Line 50 drops the TestServer.stop promise, and the failure path (Line 65–69) exits via exitProcess(1) without cleaning up browser or server resources.
If any test assertion fails (lines 18–47), the catch block executes immediately, skipping the cleanup chain at lines 48–52 entirely. This leaves the spawned Python HTTP server process and browser instance running, causing port conflicts and flaky test reruns.
🔧 Proposed fix
|> then_(_ => browser->Playwright.close)
- |> then_(_ => {
- ignore(TestServer.stop(server));
- resolve();
- })
+ |> then_(_ => TestServer.stop(server))
+ |> then_(_ => resolve())
+ |> catch(error =>
+ TestServer.stop(server)
+ |> then_(_ => reject(error))
+ )Additionally, the outer catch block (lines 65–69) should also clean up before exiting:
|> catch(error => {
Js.log2("DayPicker browser tests failed:", error);
+ // Add browser.close() and TestServer.stop(server) here before exitProcess
BrowserTestUtils.exitProcess(1);
resolve();
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@example/test-browser/DayPickerBrowserTest.re` around lines 48 - 69, The test
currently drops the TestServer.stop promise and skips cleanup on failure; update
the run() promise chain and outer catch to always await and perform cleanup:
ensure the inner finally/then branch awaits TestServer.stop(server) (don't just
ignore it) and awaits browser->Playwright.close before calling resolve, and
modify the outer catch block to first attempt to close the browser
(Playwright.close) and await TestServer.stop(server) before calling
BrowserTestUtils.exitProcess(1) and resolve; reference the existing symbols
TestServer.stop, Playwright.close, BrowserTestUtils.exitProcess, run and resolve
when making these changes so both success and failure paths always clean up and
wait for server shutdown.
| let rangeDayPicker = { | ||
| let openDateRef = ref(openDate); | ||
| let closeDateRef = ref(closeDate); | ||
| let setOpenDate = update => openDateRef := update(openDateRef^); | ||
| let setCloseDate = update => closeDateRef := update(closeDateRef^); | ||
|
|
||
| <ReactDayPicker | ||
| mode="range" | ||
| selected={`Range(Js.Undefined.return({ | ||
| ReactDayPicker.from: Js.Undefined.return(openDate), | ||
| ReactDayPicker.to_: Js.Undefined.return(closeDate), | ||
| }))} | ||
| onSelect={`Range((dates: ReactDayPicker.rangeDate) => { | ||
| switch (Js.Undefined.toOption(dates)) { | ||
| | Some(dates) => | ||
| let openDate = | ||
| switch (Js.Undefined.toOption(dates.from)) { | ||
| | Some(date) => date | ||
| | None => today | ||
| }; | ||
| let closeDate = | ||
| switch (Js.Undefined.toOption(dates.to_)) { | ||
| | Some(date) => date | ||
| | None => openDate | ||
| }; | ||
| setOpenDate(_prev => openDate); | ||
| setCloseDate(_prev => closeDate); | ||
| | None => | ||
| setOpenDate(_prev => today); | ||
| setCloseDate(_prev => today); | ||
| } | ||
| })} | ||
| footer=?footer | ||
| showOutsideDays={true} | ||
| showWeekNumber={true} | ||
| />; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files example/test-browser/DayPickerExample.reRepository: SoftwareDeployed/reason-react-day-picker
Length of output: 124
🏁 Script executed:
cat -n example/test-browser/DayPickerExample.reRepository: SoftwareDeployed/reason-react-day-picker
Length of output: 3026
Range picker selection is effectively read-only.
The selected prop (lines 50–51) always references the module-level openDate and closeDate constants, while the onSelect handler (lines 66–70) only mutates local refs. Since the component's props never change, updates to the refs do not trigger re-renders.
Replace with React state so that selection changes trigger re-renders:
🔧 Suggested fix
- let openDateRef = ref(openDate);
- let closeDateRef = ref(closeDate);
- let setOpenDate = update => openDateRef := update(openDateRef^);
- let setCloseDate = update => closeDateRef := update(closeDateRef^);
+ let (openDateState, setOpenDate) = React.useState(() => openDate);
+ let (closeDateState, setCloseDate) = React.useState(() => closeDate);
<ReactDayPicker
mode="range"
selected={`Range(Js.Undefined.return({
- ReactDayPicker.from: Js.Undefined.return(openDate),
- ReactDayPicker.to_: Js.Undefined.return(closeDate),
+ ReactDayPicker.from: Js.Undefined.return(openDateState),
+ ReactDayPicker.to_: Js.Undefined.return(closeDateState),
}))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@example/test-browser/DayPickerExample.re` around lines 41 - 77, The component
uses refs (openDateRef, closeDateRef) tied to module-level openDate/closeDate so
the selected prop never changes and the picker is effectively read-only; replace
the refs with React state (useState) inside rangeDayPicker — create [openDate,
setOpenDate] and [closeDate, setCloseDate] via useState, pass those state values
into the selected prop instead of the module constants, and update them in the
onSelect handler (use the setOpenDate/setCloseDate state setters) so re-renders
occur when the user selects a range; remove openDateRef/closeDateRef and their
update helpers once state is used.
| @@ -0,0 +1,12 @@ | |||
| <!> | |||
There was a problem hiding this comment.
Use a valid HTML doctype declaration.
Line 1 uses <!>, which is not a valid doctype and can force quirks mode in browsers.
🔧 Proposed fix
-<!>
+<!DOCTYPE html>📝 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.
| <!> | |
| <!DOCTYPE html> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@example/test-browser/index.html` at line 1, The file index.html currently
starts with an invalid doctype token "<!>"; replace this with a proper HTML5
doctype declaration by updating the top of the file to use "<!DOCTYPE html>" so
browsers render in standards mode; ensure this change is applied to the very
first line in example/test-browser/index.html before any other content.
@coderabbitai review
Summary by CodeRabbit
New Features
Documentation
Tests
Chores