Add missing Temporal built-ins and options support#277
Merged
Conversation
Owner
frostney
commented
Apr 11, 2026
frostney
commented
- Add PlainYearMonth, PlainMonthDay, and ZonedDateTime
- Extend Temporal.Now and option-bag handling
- Update docs and coverage for new Temporal APIs
- Add PlainYearMonth, PlainMonthDay, and ZonedDateTime - Extend Temporal.Now and option-bag handling - Update docs and coverage for new Temporal APIs
frostney
added
new feature
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds comprehensive Temporal support: new types (PlainYearMonth, PlainMonthDay, ZonedDateTime), TZif timezone parsing and cached UTC-offset resolution, an options-parsing module for rounding/overflow/fractional digits, many Temporal API signature updates, and a large reorganization of Temporal tests (many granular additions and several monolithic deletions). Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as rgba(30,144,255,0.5) Caller
participant Builtin as rgba(34,139,34,0.5) Temporal.Now / API
participant TZModule as rgba(255,165,0,0.5) Goccia.Temporal.TimeZone
participant Parser as rgba(220,20,60,0.5) TZif Parser / Cache
participant FS as rgba(128,0,128,0.5) Zoneinfo File System
Caller->>Builtin: Temporal.Now.zonedDateTimeISO([timeZone])
Builtin->>TZModule: GetUtcOffsetSeconds(epochSeconds, timeZone?)
TZModule->>Parser: Lookup cached TTimeZoneData for timeZoneId
alt cache miss
Parser->>FS: Read /usr/share/zoneinfo/<id> (TZif)
FS-->>Parser: TZif bytes
Parser->>Parser: Parse TZif v1/v2/v3 -> transitions, offsets
Parser-->>TZModule: Cache and return TTimeZoneData
else cache hit
Parser-->>TZModule: Return cached TTimeZoneData
end
TZModule-->>Builtin: offsetSeconds
Builtin-->>Caller: Construct ZonedDateTime with epoch±offset and timeZoneId
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
units/Goccia.Values.TemporalPlainDateTime.pas (1)
683-696:⚠️ Potential issue | 🟠 Major
roundingIncrementis ignored whensmallestUnitisday.The non-day path uses
Incrementin the divisor, but thetuDaybranch hardcodes a single-day divisor and never validates or applies the caller-supplied increment. Right nowround({ smallestUnit: "day", roundingIncrement: 2 })behaves the same as increment1.Suggested fix
if SmallestUnit = tuDay then begin + if Increment <> 1 then + ThrowRangeError('roundingIncrement must be 1 when smallestUnit is day'); // Round to nearest day using the specified rounding mode Divisor := NANOSECONDS_PER_DAY; Rounded := RoundWithMode(TotalNs, Divisor, Mode);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.TemporalPlainDateTime.pas` around lines 683 - 696, The day-branch ignores the caller's roundingIncrement: replace the hardcoded single-day divisor with one that uses the existing Increment (roundingIncrement) and validate it, i.e. compute Divisor := NANOSECONDS_PER_DAY * Increment (after asserting Increment is within the allowed range), call Rounded := RoundWithMode(TotalNs, Divisor, Mode) and use that Divisor/Rounded to decide how many days to add (add Increment days when Rounded >= Divisor) so the tuDay path behaves the same as the non-day path; update references in this block to use Increment, RoundWithMode, TotalNs, Mode, and AddDaysToDate accordingly.
🧹 Nitpick comments (5)
units/Goccia.Values.TemporalDuration.pas (1)
492-492: Add the TC39 Temporal annotation above this method.This is new spec-defined behavior, so it should keep the
// TC39 Temporal §...cross-reference immediately above the implementation.As per coding guidelines, "ECMAScript spec annotations: Add // ESYYYY §X.Y.Z SpecMethodName(specParams) comments immediately above function body in implementation section when implementing spec-defined behavior".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.TemporalDuration.pas` at line 492, Add the TC39 Temporal spec annotation comment immediately above the implementation of TGocciaTemporalDurationValue.DurationRound; specifically insert a line like "// TC39 Temporal §<section> Temporal.Duration.prototype.round(temporalDuration, options)" (use the correct § number for Duration.round) directly above the function header for DurationRound in the implementation section so the cross-reference to the spec is preserved per the coding guidelines.tests/built-ins/Temporal/plain-year-month.js (1)
8-156: Break this suite up byTemporal.PlainYearMonthmethod.This file currently tests construction, accessors, statics, arithmetic, conversion, and branding together. Splitting it by method/operation will match the existing test conventions and make regressions much easier to pinpoint.
As per coding guidelines, "JavaScript test structure: One method per file, each test file focuses on a single method or operation. Never bundle multiple methods into one file".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Temporal/plain-year-month.js` around lines 8 - 156, The suite bundles many Temporal.PlainYearMonth behaviors in one file; split tests so each file focuses on a single method/operation (e.g., constructor, calendarId, monthCode, daysInMonth, daysInYear, monthsInYear, inLeapYear, from, compare, with, add, subtract, until, since, equals, toString, toJSON, valueOf, toPlainDate, Symbol.toStringTag). For each method create a new test file using describe.runIf(isTemporal)("Temporal.PlainYearMonth.<method>") and move only the relevant test cases for that method (keeping test bodies intact) into that file, leaving shared helpers (if any) imported; ensure static tests like PlainYearMonth.from and PlainYearMonth.compare are placed in their own files named for the static method, and update any test names to reflect the single-method focus.tests/built-ins/Temporal/options-bags.js (1)
8-132: Split this into per-method Temporal test files.This suite mixes several unrelated surfaces (
round,from,toString,Now.timeZoneId) into one file, which makes failures harder to localize and goes against the repo’s Temporal test layout.As per coding guidelines, "JavaScript test structure: One method per file, each test file focuses on a single method or operation. Never bundle multiple methods into one file".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Temporal/options-bags.js` around lines 8 - 132, The test file bundles many unrelated Temporal behaviors; split each top-level describe block into its own test file so each file tests a single method/operation (e.g., create separate files for PlainTime.round, PlainDateTime.round, Instant.round, Duration.round, PlainDate.from overflow, PlainTime.toString fractionalSecondDigits, and Temporal.Now.timeZoneId). For each new file, move the corresponding describe(...) block (and its tests that reference symbols like PlainTime.round, PlainDateTime.round, Instant.round, Duration.round, PlainDate.from, PlainTime.toString, Temporal.Now.timeZoneId) into a new file, keep the outer describe.runIf(isTemporal) wrapper, preserve test names and expectations, and remove the moved blocks from the original file; ensure any shared helpers/fixtures remain imported or available to each new file and run the test suite to verify no duplicate definitions remain.tests/built-ins/Temporal/zoned-date-time.js (1)
8-189: Split theTemporal.ZonedDateTimecoverage by operation.This file groups constructor behavior, getters, arithmetic, conversions, formatting, and comparison into one suite. Keeping one operation per file will align with the repo’s test structure and reduce the blast radius of individual failures.
As per coding guidelines, "JavaScript test structure: One method per file, each test file focuses on a single method or operation. Never bundle multiple methods into one file".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Temporal/zoned-date-time.js` around lines 8 - 189, The test file bundles many Temporal.ZonedDateTime operations into one suite; split it so each file tests a single operation. Create separate test files (or suites) for constructor behavior (new Temporal.ZonedDateTime / epochNanoseconds), calendarId, epochMilliseconds/epochNanoseconds, offset/offsetNanoseconds, calendar getters (dayOfWeek, dayOfYear, daysInWeek, daysInMonth, daysInYear, monthsInYear, inLeapYear, monthCode), withTimeZone, withPlainTime, add, subtract, equals, startOfDay, toInstant, toPlainDate, toPlainTime, toPlainDateTime, formatting/toString/toJSON, valueOf (throws), compare, hoursInDay, and Symbol.toStringTag; move each corresponding test block (e.g. tests referencing Temporal.ZonedDateTime constructor, .add, .withPlainTime, .toPlainDateTime, Temporal.ZonedDateTime.compare, etc.) into its own file named/organized per operation and keep test names focused on that single method. Ensure imports/describe.runIf(isTemporal) and any shared epochNs constants are preserved or duplicated minimally per file.units/Goccia.Values.TemporalPlainMonthDay.pas (1)
179-292: Add the required ES spec annotations above the new PlainMonthDay methods.This file implements spec-defined Temporal behavior, but the getters/methods are missing the repo-standard
// ESYYYY §X.Y.Z SpecMethodName(...)comments used for spec-traceability.As per coding guidelines, "ECMAScript spec annotations: Add // ESYYYY §X.Y.Z SpecMethodName(specParams) comments immediately above function body in implementation section when implementing spec-defined behavior".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.TemporalPlainMonthDay.pas` around lines 179 - 292, Add the required ECMAScript spec annotation comments immediately above each implementation of the spec-defined PlainMonthDay routines: GetCalendarId, GetMonthCode, GetDay, MonthDayWith, MonthDayEquals, MonthDayToString, MonthDayToJSON, MonthDayValueOf, and MonthDayToPlainDate; use the repo-standard format "// ESYYYY §X.Y.Z SpecMethodName(specParams)" (replace ESYYYY/§X.Y.Z/specParams with the correct spec reference and signature) and place each comment directly above the corresponding function header in the implementation section so each method is traceable to the spec.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@units/Goccia.Builtins.Temporal.pas`:
- Around line 53-56: PlainMonthDay is missing its static compare implementation;
add a function PlainMonthDayCompare(const AArgs: TGocciaArgumentsCollection;
const AThisValue: TGocciaValue): TGocciaValue that implements the same compare
semantics used by PlainYearMonth/ZonedDateTime compare helpers, then register
that function on the PlainMonthDay constructor statics alongside
PlainMonthDayFrom so the exported API includes PlainMonthDay.compare (mirror the
pattern used for PlainYearMonth and ZonedDateTime).
- Around line 496-509: The code calls Obj.GetProperty('year'/'month'/'day') and
immediately uses ToNumberLiteral, which will access nil if the property is
missing; update the blocks around TGocciaObjectValue/Obj.GetProperty usages
(including the shown block that constructs TGocciaTemporalPlainDateValue.Create
and the other occurrences at 875-878 and 979-1000) to guard each GetProperty
result: if the returned value is nil or represents undefined, raise a JS
TypeError (use the same error reporting pattern as ThrowRangeError) before
calling ToNumberLiteral; keep existing overflow handling (Overflow/toReject and
ThrowRangeError) and only proceed to Trunc/DaysInMonth and Create after
validating year, month, and day are present and numeric.
- Around line 1067-1083: The code currently overwrites an empty TZ with 'UTC',
which converts offset-only (zone-less) ISO strings into UTC-zoned values;
instead, when TZ = '' do not assign 'UTC'—reject unannotated/offset-only inputs
to match CoerceZonedDateTime behavior. Modify the block after parsing (in the
routine using TryParseISODateTimeWithOffset and creating
TGocciaTemporalZonedDateTimeValue) to remove the TZ := 'UTC' assignment and call
ThrowRangeError (or otherwise fail) if TZ = '' so you do not fabricate a UTC
zone for zone-less strings.
In `@units/Goccia.Temporal.Options.pas`:
- Around line 268-349: The midpoint logic uses HalfDivisor := ADivisor div 2
which truncates for odd ADivisor and misclassifies half ties; replace the
HalfDivisor-based checks with comparisons that double the remainder instead
(e.g. use AbsRemainder * 2 < ADivisor, = ADivisor, > ADivisor or >=/<= as
appropriate) across all rmHalf* branches (symbols: AbsRemainder, ADivisor,
AMode, rmHalfExpand, rmHalfTrunc, rmHalfCeil, rmHalfFloor, rmHalfEven) so ties
are determined by AbsRemainder * 2 = ADivisor and "greater than half" by
AbsRemainder * 2 > ADivisor; remove or stop using HalfDivisor to avoid the
off-by-one on odd divisors.
In `@units/Goccia.Temporal.TimeZone.pas`:
- Around line 257-259: ATimeZone is concatenated into ZONEINFO_PATH to form
FilePath and can contain malicious segments (e.g., "../" or absolute paths) that
escape the zoneinfo directory; before using FilePath, validate and normalize
ATimeZone by rejecting empty, absolute paths, and any '.' or '..' path segments
and ensure it does not contain path separators that would traverse up (normalize
by splitting on path separators and rejecting any segment equal to '.' or '..');
then build FilePath as ZONEINFO_PATH + validated ATimeZone and only then call
FileExists or TFileStream.Create (update the code paths referenced by FilePath,
ZONEINFO_PATH, ATimeZone, FileExists, and TFileStream.Create accordingly).
- Around line 114-150: The code advances offsets by TZIF_MAGIC_LENGTH extra
bytes when skipping the v1 data and when locating the v2 header, causing reads
to be 4 bytes too far; update the offset calculations in the parsing flow (the
block that computes Offset before skipping v1 data and the similar reuse around
the v2 header check in the same routine) to use TZIF_HEADER_SIZE alone (do not
add TZIF_MAGIC_LENGTH) so that Offset := TZIF_HEADER_SIZE + V1DataSize (or
equivalently start from the existing TZIF_MAGIC_LENGTH+TZIF_HEADER_SIZE base
only once where appropriate) and apply the same correction for the second
occurrence referenced around the 183-186 range to ensure subsequent reads
(ReadBigEndianInt32, transition parsing) use the correct positions.
In `@units/Goccia.Temporal.Utils.pas`:
- Around line 813-827: The offset parsing currently ignores TryParseDigits in
the compact-offset branch and fails to validate ranges or trailing characters;
update the block that reads OffsetPart (variables OffsetPart, ParsePos, OffsetH,
OffsetM, AOffsetSeconds and function TryParseDigits) to (1) check the boolean
return from TryParseDigits when parsing the compact form and Exit on failure,
(2) validate OffsetM is in 0..59 and OffsetH is in an appropriate non-negative
range (reject out-of-range values), and (3) ensure there are no extra trailing
characters by confirming ParsePos has consumed the entire OffsetPart before
computing AOffsetSeconds; if any check fails, Exit instead of normalizing to
:00.
In `@units/Goccia.Values.TemporalDuration.pas`:
- Around line 533-600: The rounding logic incorrectly overwrites LargestUnit
with SmallestUnit and drops weeks; revert the assignment so LargestUnit remains
"auto" (don’t set LargestUnit := SmallestUnit), implement reconstruction of
weeks when breaking RemNs into components (add ResultWeeks variable, compute it
using NANOSECONDS_PER_DAY*7 when Ord(LargestUnit) <= Ord(tuWeek) before days,
include D.FWeeks in TotalNs handling already present), and pass ResultWeeks into
TGocciaTemporalDurationValue.Create instead of the hardcoded 0; also add the
required ECMA-262/Temporal spec annotation comment immediately before this
implementation block to document spec-defined behavior.
In `@units/Goccia.Values.TemporalPlainDate.pas`:
- Around line 583-597: The conversions in DateToPlainYearMonth and
DateToPlainMonthDay drop the source date's hidden reference fields (D.FDay and
D.FYear); update DateToPlainYearMonth
(TGocciaTemporalPlainDateValue.DateToPlainYearMonth) to create the
TGocciaTemporalPlainYearMonthValue with the original reference day (preserve
D.FDay) and update DateToPlainMonthDay
(TGocciaTemporalPlainDateValue.DateToPlainMonthDay) to create the
TGocciaTemporalPlainMonthDayValue with the original reference year (preserve
D.FYear) — do this by using the constructor overloads that accept the reference
fields or by setting the appropriate reference properties on the returned
TGocciaTemporalPlainYearMonthValue / TGocciaTemporalPlainMonthDayValue instances
after creation so the hidden reference metadata from D is retained.
In `@units/Goccia.Values.TemporalPlainDateTime.pas`:
- Around line 769-783: DateTimeToPlainYearMonth and DateTimeToPlainMonthDay drop
original date metadata because they call AsPlainDateTime then construct
PlainYearMonth/PlainMonthDay without passing reference day/year; update
DateTimeToPlainYearMonth (function
TGocciaTemporalPlainDateTimeValue.DateTimeToPlainYearMonth) to pass D.FDay into
the PlainYearMonth creation (use the overload that accepts a reference day) and
update DateTimeToPlainMonthDay (function
TGocciaTemporalPlainDateTimeValue.DateTimeToPlainMonthDay) to pass D.FYear into
the PlainMonthDay creation (use the overload that accepts a reference year),
keeping the call to AsPlainDateTime as-is so the original
D.FYear/D.FMonth/D.FDay are preserved when constructing
TGocciaTemporalPlainYearMonthValue and TGocciaTemporalPlainMonthDayValue.
In `@units/Goccia.Values.TemporalZonedDateTime.pas`:
- Around line 887-946: The round implementation only reads UnitStr and always
does half-expand with increment 1; fix by reading and validating roundingMode
and roundingIncrement from the options object (when Arg is TGocciaObjectValue)
using the shared option-bag helpers, defaulting roundingMode to 'halfExpand' and
roundingIncrement to 1, then compute Divisor as NANOSECONDS_PER_<unit> *
roundingIncrement (e.g. for minute: NANOSECONDS_PER_MINUTE * roundingIncrement)
and apply the correct rounding behavior based on roundingMode ('trunc' =>
truncate/floor, 'ceil' => ceiling, 'halfExpand' => current half-up logic using
Divisor), updating how Rounded is computed before calling BalanceTime; refer to
UnitStr, Divisor, roundingMode, roundingIncrement, TotalNs, Rounded and Balanced
to locate where to add extraction/validation and to replace the
always-half-expand logic.
- Around line 511-515: GetHoursInDay currently returns the constant
HOURS_PER_DAY which is wrong for days with DST shifts; instead use
AsZonedDateTime to get the TGocciaTemporalZonedDateTimeValue's zoned datetime
instance, compute the start of that calendar day (set time to 00:00) and the
start of the following day in the same zone, measure the difference in hours
between those two instants (accounting for zone offsets/DST), and return
TGocciaNumberLiteralValue.Create(calculatedHours) rather than HOURS_PER_DAY.
- Around line 711-726: The code in the ZonedDateTime withPlainTime handling
currently leaves NewHour/NewMinute/etc. at zero when Arg is non-undefined but
not a TGocciaTemporalPlainTimeValue or TGocciaStringLiteralValue, silently
changing the instant; update the branch so that after checking Assigned(Arg) and
not TGocciaUndefinedLiteralValue, if Arg is TGocciaTemporalPlainTimeValue set
New* from PlainTime, else if Arg is TGocciaStringLiteralValue parse via
TryParseISOTime, else call ThrowTypeError('Invalid time string for
ZonedDateTime.prototype.withPlainTime') (or a clearer message) to reject
unsupported plainTimeLike inputs; adjust the logic around
TGocciaTemporalPlainTimeValue, TGocciaStringLiteralValue, TryParseISOTime and
ThrowTypeError accordingly.
---
Outside diff comments:
In `@units/Goccia.Values.TemporalPlainDateTime.pas`:
- Around line 683-696: The day-branch ignores the caller's roundingIncrement:
replace the hardcoded single-day divisor with one that uses the existing
Increment (roundingIncrement) and validate it, i.e. compute Divisor :=
NANOSECONDS_PER_DAY * Increment (after asserting Increment is within the allowed
range), call Rounded := RoundWithMode(TotalNs, Divisor, Mode) and use that
Divisor/Rounded to decide how many days to add (add Increment days when Rounded
>= Divisor) so the tuDay path behaves the same as the non-day path; update
references in this block to use Increment, RoundWithMode, TotalNs, Mode, and
AddDaysToDate accordingly.
---
Nitpick comments:
In `@tests/built-ins/Temporal/options-bags.js`:
- Around line 8-132: The test file bundles many unrelated Temporal behaviors;
split each top-level describe block into its own test file so each file tests a
single method/operation (e.g., create separate files for PlainTime.round,
PlainDateTime.round, Instant.round, Duration.round, PlainDate.from overflow,
PlainTime.toString fractionalSecondDigits, and Temporal.Now.timeZoneId). For
each new file, move the corresponding describe(...) block (and its tests that
reference symbols like PlainTime.round, PlainDateTime.round, Instant.round,
Duration.round, PlainDate.from, PlainTime.toString, Temporal.Now.timeZoneId)
into a new file, keep the outer describe.runIf(isTemporal) wrapper, preserve
test names and expectations, and remove the moved blocks from the original file;
ensure any shared helpers/fixtures remain imported or available to each new file
and run the test suite to verify no duplicate definitions remain.
In `@tests/built-ins/Temporal/plain-year-month.js`:
- Around line 8-156: The suite bundles many Temporal.PlainYearMonth behaviors in
one file; split tests so each file focuses on a single method/operation (e.g.,
constructor, calendarId, monthCode, daysInMonth, daysInYear, monthsInYear,
inLeapYear, from, compare, with, add, subtract, until, since, equals, toString,
toJSON, valueOf, toPlainDate, Symbol.toStringTag). For each method create a new
test file using describe.runIf(isTemporal)("Temporal.PlainYearMonth.<method>")
and move only the relevant test cases for that method (keeping test bodies
intact) into that file, leaving shared helpers (if any) imported; ensure static
tests like PlainYearMonth.from and PlainYearMonth.compare are placed in their
own files named for the static method, and update any test names to reflect the
single-method focus.
In `@tests/built-ins/Temporal/zoned-date-time.js`:
- Around line 8-189: The test file bundles many Temporal.ZonedDateTime
operations into one suite; split it so each file tests a single operation.
Create separate test files (or suites) for constructor behavior (new
Temporal.ZonedDateTime / epochNanoseconds), calendarId,
epochMilliseconds/epochNanoseconds, offset/offsetNanoseconds, calendar getters
(dayOfWeek, dayOfYear, daysInWeek, daysInMonth, daysInYear, monthsInYear,
inLeapYear, monthCode), withTimeZone, withPlainTime, add, subtract, equals,
startOfDay, toInstant, toPlainDate, toPlainTime, toPlainDateTime,
formatting/toString/toJSON, valueOf (throws), compare, hoursInDay, and
Symbol.toStringTag; move each corresponding test block (e.g. tests referencing
Temporal.ZonedDateTime constructor, .add, .withPlainTime, .toPlainDateTime,
Temporal.ZonedDateTime.compare, etc.) into its own file named/organized per
operation and keep test names focused on that single method. Ensure
imports/describe.runIf(isTemporal) and any shared epochNs constants are
preserved or duplicated minimally per file.
In `@units/Goccia.Values.TemporalDuration.pas`:
- Line 492: Add the TC39 Temporal spec annotation comment immediately above the
implementation of TGocciaTemporalDurationValue.DurationRound; specifically
insert a line like "// TC39 Temporal §<section>
Temporal.Duration.prototype.round(temporalDuration, options)" (use the correct §
number for Duration.round) directly above the function header for DurationRound
in the implementation section so the cross-reference to the spec is preserved
per the coding guidelines.
In `@units/Goccia.Values.TemporalPlainMonthDay.pas`:
- Around line 179-292: Add the required ECMAScript spec annotation comments
immediately above each implementation of the spec-defined PlainMonthDay
routines: GetCalendarId, GetMonthCode, GetDay, MonthDayWith, MonthDayEquals,
MonthDayToString, MonthDayToJSON, MonthDayValueOf, and MonthDayToPlainDate; use
the repo-standard format "// ESYYYY §X.Y.Z SpecMethodName(specParams)" (replace
ESYYYY/§X.Y.Z/specParams with the correct spec reference and signature) and
place each comment directly above the corresponding function header in the
implementation section so each method is traceable to the spec.
🪄 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: CHILL
Plan: Pro
Run ID: de97cee3-4d22-438a-b6e6-2b1d8b0ce417
📒 Files selected for processing (22)
AGENTS.mddocs/built-ins.mdtests/built-ins/Temporal/now.jstests/built-ins/Temporal/options-bags.jstests/built-ins/Temporal/plain-date-time.jstests/built-ins/Temporal/plain-date.jstests/built-ins/Temporal/plain-month-day.jstests/built-ins/Temporal/plain-year-month.jstests/built-ins/Temporal/toStringTag.jstests/built-ins/Temporal/zoned-date-time.jsunits/Goccia.Builtins.Temporal.pasunits/Goccia.Temporal.Options.pasunits/Goccia.Temporal.TimeZone.pasunits/Goccia.Temporal.Utils.pasunits/Goccia.Values.TemporalDuration.pasunits/Goccia.Values.TemporalInstant.pasunits/Goccia.Values.TemporalPlainDate.pasunits/Goccia.Values.TemporalPlainDateTime.pasunits/Goccia.Values.TemporalPlainMonthDay.pasunits/Goccia.Values.TemporalPlainTime.pasunits/Goccia.Values.TemporalPlainYearMonth.pasunits/Goccia.Values.TemporalZonedDateTime.pas
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results364 benchmarks Interpreted: 🟢 84 improved · 🔴 163 regressed · 117 unchanged · avg -0.7% arraybuffer.js — Interp: 🟢 6, 🔴 2, 6 unch. · avg +0.8% · Bytecode: 🟢 9, 🔴 2, 3 unch. · avg +5.0%
arrays.js — Interp: 🟢 15, 🔴 1, 3 unch. · avg +3.1% · Bytecode: 🟢 18, 1 unch. · avg +8.7%
async-await.js — Interp: 🟢 1, 🔴 1, 4 unch. · avg -0.2% · Bytecode: 🟢 6 · avg +8.4%
base64.js — Interp: 🟢 6, 🔴 3, 1 unch. · avg +1.6% · Bytecode: 🟢 7, 3 unch. · avg +7.5%
classes.js — Interp: 🟢 8, 🔴 4, 19 unch. · avg +0.4% · Bytecode: 🟢 9, 🔴 4, 18 unch. · avg +1.9%
closures.js — Interp: 🟢 2, 🔴 3, 6 unch. · avg +0.3% · Bytecode: 🟢 2, 🔴 4, 5 unch. · avg -1.4%
collections.js — Interp: 🟢 5, 🔴 5, 2 unch. · avg -0.2% · Bytecode: 🟢 9, 🔴 2, 1 unch. · avg +3.1%
destructuring.js — Interp: 🟢 12, 🔴 5, 5 unch. · avg +1.0% · Bytecode: 🟢 1, 🔴 15, 6 unch. · avg -5.2%
fibonacci.js — Interp: 🔴 6, 2 unch. · avg -2.6% · Bytecode: 🟢 4, 🔴 2, 2 unch. · avg +2.4%
float16array.js — Interp: 🟢 3, 🔴 13, 16 unch. · avg -1.3% · Bytecode: 🟢 8, 🔴 17, 7 unch. · avg -6.2%
for-of.js — Interp: 🟢 2, 🔴 3, 2 unch. · avg +0.1% · Bytecode: 🟢 2, 🔴 2, 3 unch. · avg -1.5%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 2, 🔴 27, 13 unch. · avg -3.0% · Bytecode: 🟢 11, 🔴 20, 11 unch. · avg -1.4%
json.js — Interp: 🔴 18, 2 unch. · avg -3.3% · Bytecode: 🟢 11, 🔴 1, 8 unch. · avg +2.8%
jsx.jsx — Interp: 🔴 17, 4 unch. · avg -2.0% · Bytecode: 🟢 6, 🔴 3, 12 unch. · avg +0.8%
modules.js — Interp: 🟢 2, 🔴 2, 5 unch. · avg +0.4% · Bytecode: 🟢 9 · avg +6.2%
numbers.js — Interp: 🟢 1, 🔴 4, 6 unch. · avg -0.6% · Bytecode: 🟢 6, 🔴 3, 2 unch. · avg +4.0%
objects.js — Interp: 🟢 2, 🔴 4, 1 unch. · avg -1.0% · Bytecode: 🟢 2, 🔴 1, 4 unch. · avg +1.5%
promises.js — Interp: 🔴 10, 2 unch. · avg -1.9% · Bytecode: 🟢 2, 🔴 2, 8 unch. · avg -0.3%
regexp.js — Interp: 🔴 11 · avg -3.1% · Bytecode: 🟢 2, 🔴 5, 4 unch. · avg -0.7%
strings.js — Interp: 🟢 9, 🔴 6, 4 unch. · avg +0.7% · Bytecode: 🟢 7, 🔴 1, 11 unch. · avg +1.7%
typed-arrays.js — Interp: 🟢 2, 🔴 12, 8 unch. · avg -1.6% · Bytecode: 🟢 5, 🔴 9, 8 unch. · avg -4.4%
uint8array-encoding.js — Interp: 🟢 6, 🔴 6, 6 unch. · avg +0.4% · Bytecode: 🟢 5, 🔴 10, 3 unch. · avg -3.5%
Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
…thod-per-file Address 14 CodeRabbit review findings: - Fix TZif offset off-by-4 from redundant TZIF_MAGIC_LENGTH additions - Add path traversal validation in timezone name handling - Fix half-rounding truncation for odd divisors (AbsRemainder*2 comparison) - Fix nil dereference in PlainDate/YearMonth/MonthDay.from property access - Fix ISO offset parsing to validate ranges and reject trailing characters - Fix Duration.round largestUnit default to largest defined unit - Add week support in Duration.round result - Preserve reference fields in PlainDate→PlainYearMonth/PlainMonthDay conversions - Fix PlainDateTime.round day increment computation - Add PlainMonthDay.compare static method - Fix ZonedDateTime.from to throw on zone-less strings instead of defaulting UTC - Fix ZonedDateTime.round to read roundingMode/roundingIncrement from options - Fix ZonedDateTime.withPlainTime to throw on unsupported argument types - Compute actual hoursInDay via epoch difference instead of constant 24 Add TC39 Temporal spec annotations to PlainMonthDay methods. Restructure tests from 10 monolithic files to 136 one-method-per-file tests following project conventions (prototype/ subdirectories, edge cases co-located). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
frostney
commented
Apr 12, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (16)
tests/built-ins/Temporal/PlainDate/compare.js (1)
9-15: Expandcompare()coverage with edge/error cases in this same file.This currently validates only PlainDate-instance happy paths. Please add edge/error assertions (e.g., ISO-string inputs and invalid-input throws) in this file so method coverage stays complete and co-located.
Suggested test additions
describe.runIf(isTemporal)("Temporal.PlainDate.compare", () => { test("compare()", () => { const d1 = new Temporal.PlainDate(2024, 1, 1); const d2 = new Temporal.PlainDate(2024, 6, 1); expect(Temporal.PlainDate.compare(d1, d2)).toBe(-1); expect(Temporal.PlainDate.compare(d2, d1)).toBe(1); expect(Temporal.PlainDate.compare(d1, d1)).toBe(0); }); + + test("compare() accepts ISO date strings", () => { + expect(Temporal.PlainDate.compare("2024-01-01", "2024-01-02")).toBe(-1); + expect(Temporal.PlainDate.compare("2024-01-02", "2024-01-01")).toBe(1); + expect(Temporal.PlainDate.compare("2024-01-01", "2024-01-01")).toBe(0); + }); + + test("compare() throws for invalid input", () => { + expect(() => Temporal.PlainDate.compare("not-a-date", "2024-01-01")).toThrow(); + }); });As per coding guidelines, edge case tests should live in the same file as happy-path tests for each method.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Temporal/PlainDate/compare.js` around lines 9 - 15, Add edge-case assertions to the existing compare() test in tests/built-ins/Temporal/PlainDate/compare.js: call Temporal.PlainDate.compare with ISO date strings (e.g., "2024-01-01" and "2024-06-01") and assert the same -1/1/0 results as the PlainDate instances, and add assertions that invalid inputs (e.g., null, an object, or a non-ISO string) throw a TypeError when passed to Temporal.PlainDate.compare; update the single "compare()" test to include these ISO-string happy-path checks and the invalid-input throws so all cases are co-located with the existing Temporal.PlainDate.compare(d1, d2) assertions.tests/built-ins/Temporal/PlainDateTime/prototype/valueOf.js (1)
11-11: Optional: assert thevalueOfTypeError message for stricter spec lock-in.The type check is correct; adding message matching would better guard against future regressions (implementation currently throws a specific message in
units/Goccia.Values.TemporalPlainDateTime.pas, Line 742-744).Suggested tweak
- expect(() => dt.valueOf()).toThrow(TypeError); + expect(() => dt.valueOf()).toThrow(TypeError); + expect(() => dt.valueOf()).toThrow( + "Temporal.PlainDateTime.prototype.valueOf cannot be used; use toString or compare instead" + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Temporal/PlainDateTime/prototype/valueOf.js` at line 11, The test currently only asserts that dt.valueOf() throws a TypeError; update the assertion to also verify the exact error message thrown by valueOf so the test locks to the implementation message from units/Goccia.Values.TemporalPlainDateTime.pas (the message emitted around lines 742–744). Replace or extend the current expect(() => dt.valueOf()).toThrow(TypeError) with a message-aware assertion (e.g., the test harness's toThrow/toThrowError variant that accepts a message or regex) and use the exact string used in the implementation to ensure strict matching.tests/built-ins/Temporal/Instant/from.js (2)
9-13: Strengthen the Instant-input case with a non-alias assertion.Line 12 checks value equality, but it doesn’t verify object identity behavior. Add
expect(i2).not.toBe(i1)to ensure this path is validated as a copy flow, not passthrough.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Temporal/Instant/from.js` around lines 9 - 13, The Instant.from test currently only asserts epoch milliseconds equality for the Instant-input path; add an identity assertion to ensure Instant.from produces a distinct object. In the "from() with Instant" test that creates i1 via Temporal.Instant.fromEpochMilliseconds and i2 via Temporal.Instant.from(i1), insert an assertion expect(i2).not.toBe(i1) (in addition to the existing epochMilliseconds check) to validate the copy-not-alias behavior of Instant.from.
15-18: Add edge/error coverage forTemporal.Instant.fromin this file.You already have happy paths; add invalid-input/error cases here (e.g., unsupported type, malformed string) to keep method coverage complete in one place.
Proposed test additions
describe.runIf(isTemporal)("Temporal.Instant.from", () => { test("from() with Instant", () => { const i1 = Temporal.Instant.fromEpochMilliseconds(5000); const i2 = Temporal.Instant.from(i1); + expect(i2).not.toBe(i1); expect(i2.epochMilliseconds).toBe(5000); }); test("from() with ISO string", () => { const instant = Temporal.Instant.from("2024-01-15T12:30:45Z"); expect(instant.epochMilliseconds).toBe(1705321845000); }); + + test("from() throws on unsupported input type", () => { + expect(() => Temporal.Instant.from(123)).toThrow(); + }); });As per coding guidelines: “Edge case tests (...) belong in the same file as happy-path tests for that method.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Temporal/Instant/from.js` around lines 15 - 18, Add negative test cases for Temporal.Instant.from to cover invalid inputs: add tests that call Temporal.Instant.from with a malformed ISO string (e.g., "not-a-date") and with unsupported types (e.g., an object, null, or a non-string/non-temporal value) and assert that each call throws a RangeError/TypeError as appropriate; locate calls to Temporal.Instant.from in this file and create new tests named like "from() with malformed string" and "from() with unsupported type" that use expect(() => Temporal.Instant.from(...)).toThrow() to verify error behavior.tests/built-ins/Temporal/Instant/constructor.js (1)
9-12: Add edge/error cases in this same method-focused file.This file currently has only a happy-path check. Add at least one boundary/error case here (e.g., invalid input handling) so the method coverage is complete in-place.
Based on learnings: “tests should cover happy paths, edge cases, and error cases — all in the same file for each method.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Temporal/Instant/constructor.js` around lines 9 - 12, Add edge and error-case tests to the existing "epochNanoseconds getter" test file by extending or adding tests around Temporal.Instant.fromEpochMilliseconds and the epochNanoseconds property: include at least one invalid-input case (e.g., pass a non-numeric value like a string or NaN and assert it throws), and a boundary case (e.g., very large or negative epoch milliseconds to assert correct epochNanoseconds or an out-of-range error). Locate the existing test block using the test name "epochNanoseconds getter" and the Temporal.Instant.fromEpochMilliseconds call, and add assertions that expect throws for invalid inputs and precise conversions for boundary values to ensure full method coverage.tests/built-ins/Temporal/Duration/prototype/total.js (1)
9-25: Add options-bag test fortotal()to ensure option parsing doesn't regress.The current tests only exercise the string overload (
d.total("minutes")). The Temporal API also accepts an options object (d.total({ unit: "minutes" })). Add at least one options-bag call to catch regressions in option handling for this method.Suggested test addition
test("total()", () => { const d = new Temporal.Duration(0, 0, 0, 0, 1, 30); expect(d.total("minutes")).toBe(90); expect(d.total("hours")).toBe(1.5); expect(d.total("seconds")).toBe(5400); + expect(d.total({ unit: "minutes" })).toBe(90); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Temporal/Duration/prototype/total.js` around lines 9 - 25, Add a test that calls Temporal.Duration.prototype.total using the options-bag form (e.g., d.total({ unit: "minutes" })) to ensure option parsing is exercised; update the tests in tests/built-ins/Temporal/Duration/prototype/total.js to include at least one assertion using the object overload (for example mirroring an existing string-based assertion like expect(d.total({ unit: "minutes" })).toBe(90)) and optionally add a corresponding RangeError assertion that uses the options object (e.g., expect(() => withYears.total({ unit: "days" })).toThrow(RangeError)).units/Goccia.Temporal.TimeZone.pas (2)
126-133: Consider extracting magic offset20to a named constant.The offset
20appears to be the byte position where TZif header fields begin (after magic + version + reserved bytes). Extracting this to a named constant would clarify the intent.const TZIF_MAGIC = 'TZif'; TZIF_MAGIC_LENGTH = 4; TZIF_HEADER_SIZE = 44; + TZIF_HEADER_FIELDS_OFFSET = 20; ... - Offset := 20; + Offset := TZIF_HEADER_FIELDS_OFFSET;As per coding guidelines: "Extract bare numeric literals in
implementationsections into named constants so the value is defined once and the name conveys intent."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Temporal.TimeZone.pas` around lines 126 - 133, Extract the magic offset 20 into a named constant (e.g. TZIFHeaderOffset or HeaderStartOffset) declared in the implementation section and replace the literal in the line "Offset := 20;" with that constant; update any related comments to reference the constant and leave the subsequent ReadBigEndianInt32 calls (IsUtCnt, IsStdCnt, LeapCnt, TimeCnt, TypeCnt, CharCnt) unchanged so the intent that the header starts after magic+version+reserved bytes is clear.
288-307: Resource management could usetry..finallyfor clarity.The current pattern manually frees
Streamin multiple places (lines 294, 300, 305). Atry..finallyblock would be cleaner and safer:♻️ Suggested refactor
Stream := nil; try Stream := TFileStream.Create(FilePath, fmOpenRead or fmShareDenyNone); - FileSize := Stream.Size; - if FileSize < TZIF_MAGIC_LENGTH + TZIF_HEADER_SIZE then - begin - Stream.Free; - Exit; - end; - - SetLength(Buffer, FileSize); - Stream.ReadBuffer(Buffer[0], FileSize); - Stream.Free; - Stream := nil; - - Result := ParseTZifData(ATimeZone, Buffer, FileSize, AData); - except - Stream.Free; + try + FileSize := Stream.Size; + if FileSize < TZIF_MAGIC_LENGTH + TZIF_HEADER_SIZE then + Exit; + SetLength(Buffer, FileSize); + Stream.ReadBuffer(Buffer[0], FileSize); + Result := ParseTZifData(ATimeZone, Buffer, FileSize, AData); + finally + Stream.Free; + end; + except + // Silently fail - Result remains False end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Temporal.TimeZone.pas` around lines 288 - 307, Replace manual Stream.Free calls with a try..finally to guarantee Stream is freed; create the TFileStream (TFileStream.Create) before a try, perform FileSize checks, SetLength(Buffer) and Stream.ReadBuffer(Buffer[0], FileSize) inside the try, then in the finally call Stream.Free or FreeAndNil(Stream) if assigned. Remove the extra Stream.Free calls inside the if and except blocks and ensure the function still calls ParseTZifData(ATimeZone, Buffer, FileSize, AData) after reading; reference symbols: TFileStream.Create, Stream.ReadBuffer, ParseTZifData, Stream.Free/FreeAndNil.tests/built-ins/Temporal/ZonedDateTime/prototype/withPlainTime.js (1)
1-20: LGTM! Consider adding edge case coverage.The test correctly validates that
withPlainTimepreserves the date portion while setting the time components. The guard pattern and structure follow repository conventions.Per coding guidelines, edge cases belong in the same file. Consider adding tests for:
- Calling with
undefined(should reset to midnight per TC39 spec)- Calling with a time string instead of
PlainTimeobject🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Temporal/ZonedDateTime/prototype/withPlainTime.js` around lines 1 - 20, Add two edge-case tests to tests/built-ins/Temporal/ZonedDateTime/prototype/withPlainTime.js: one that calls ZonedDateTime.prototype.withPlainTime with undefined (e.g., zdt.withPlainTime(undefined)) and asserts the time resets to midnight (hour/minute/second = 0) while date remains unchanged, and another that passes a time string (e.g., "00:00" or "12:34:56") to withPlainTime and asserts the resulting hour/minute/second reflect that parsed time while year/month/day remain unchanged; locate the existing test using the zdt variable and function under test (withPlainTime / Temporal.ZonedDateTime) and follow the same describe.runIf(isTemporal) / test structure and assertion style as the existing cases.tests/built-ins/Temporal/PlainYearMonth/prototype/toPlainDate.js (1)
1-16: LGTM! Consider edge case for day overflow.The test correctly validates that
toPlainDate({ day: 15 })combines year/month with the provided day.Per coding guidelines, edge cases should be in the same file. Consider testing behavior when
dayexceeds the month's days (e.g.,{ day: 31 }for February) — the implementation passes the day directly to thePlainDateconstructor which may clamp or throw.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Temporal/PlainYearMonth/prototype/toPlainDate.js` around lines 1 - 16, Add an edge-case test in the same file that verifies Temporal.PlainYearMonth.prototype.toPlainDate behavior when the provided day overflows the month: create a PlainYearMonth (e.g., ym = new Temporal.PlainYearMonth(2024, 2)) and assert that calling ym.toPlainDate({ day: 31 }) throws (use expect(() => ym.toPlainDate({ day: 31 })).toThrow() or the appropriate RangeError expectation) so the test documents whether toPlainDate rejects out-of-range days.tests/built-ins/Temporal/Now/zonedDateTimeISO.js (1)
9-15: Expand coverage to include argument and error paths.This currently checks only the no-arg happy path. Add an explicit
timeZoneargument case and one invalid-input/error case in this file to harden the newNow.zonedDateTimeISObehavior.As per coding guidelines, “tests should cover happy paths, edge cases, and error cases — all in the same file for each method.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Temporal/Now/zonedDateTimeISO.js` around lines 9 - 15, Add two new tests to tests/built-ins/Temporal/Now/zonedDateTimeISO.js: one that calls Temporal.Now.zonedDateTimeISO("America/New_York") (or another valid IANA zone) and asserts the returned object is a ZonedDateTime with a non-empty timeZoneId equal to the provided zone or that its timeZoneId includes the expected zone identifier and epochMilliseconds > 0; and another that calls Temporal.Now.zonedDateTimeISO with an invalid argument (e.g., a number or "Invalid/Zone") and asserts that it throws the appropriate error (RangeError or TypeError) using expect(() => Temporal.Now.zonedDateTimeISO(badArg)).toThrow(); reference the Temporal.Now.zonedDateTimeISO call site in the file when adding these tests.tests/built-ins/Temporal/ZonedDateTime/prototype/add.js (1)
9-15: Add boundary/error coverage in this method file.Line 12 currently validates only a simple same-day add. Please add one boundary case (e.g., crossing day/month) plus one invalid-duration/error case in this same file.
As per coding guidelines, “tests should cover happy paths, edge cases, and error cases — all in the same file for each method.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Temporal/ZonedDateTime/prototype/add.js` around lines 9 - 15, Add two new tests in the same file alongside the existing "add duration" test: one boundary test that uses a ZonedDateTime near the end of a day/month (e.g., create zdt at 23:30 on Jan 31 or just before midnight) and call zdt.add with a duration that crosses into the next day/month, then assert the expected hour/day/month values; and one error test that calls zdt.add with an invalid duration (e.g., a non-object, NaN, or a disallowed property) and asserts it throws (use expect(() => zdt.add(...)).toThrow()). Reference the existing variable and method names (zdt, ZonedDateTime, zdt.add) so the new tests match style and placement with the current "add duration" test.tests/built-ins/Temporal/ZonedDateTime/prototype/startOfDay.js (1)
8-19: Add an in-file timezone edge case forstartOfDay().Line 11 only validates the UTC happy path. Please add at least one non-UTC boundary case (e.g., DST transition) and keep it in this same file so method coverage includes edge behavior.
As per coding guidelines, “Edge case tests (NaN, Infinity, negative indices, clamping, empty collections, boundary conditions) belong in the same file as happy-path tests for that method.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Temporal/ZonedDateTime/prototype/startOfDay.js` around lines 8 - 19, Add a non‑UTC boundary test for Temporal.ZonedDateTime.prototype.startOfDay in the same file: create a second test (e.g., "startOfDay DST/boundary") that constructs a ZonedDateTime (zdt) in a DST‑observing zone such as "America/New_York" at an instant that lies on or around a DST transition, call zdt.startOfDay() to get sod, and assert the expected hour/minute/second/year/month/day for that local start of day so the edge behaviour (clock jump/back) is covered alongside the existing UTC happy path.tests/built-ins/Temporal/PlainDate/prototype/add.js (1)
9-21: Good rollover coverage; add one invalid-input assertion.Lines 10–20 cover happy + boundary behavior well. Add one explicit error-path test (invalid duration-like input) to complete per-method coverage in this file.
As per coding guidelines, “tests should cover happy paths, edge cases, and error cases — all in the same file for each method.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Temporal/PlainDate/prototype/add.js` around lines 9 - 21, Add one error-path test for Temporal.PlainDate.prototype.add that asserts a TypeError is thrown for an invalid duration-like input: create a PlainDate (e.g. new Temporal.PlainDate(2024,1,1)) and call its add method with an invalid value (e.g. a plain string or an object with wrong-typed fields), then use expect(() => d.add(...)).toThrow(TypeError) — name the test "add() with invalid duration" to match existing tests.tests/built-ins/Temporal/ZonedDateTime/prototype/subtract.js (1)
9-15: Please add underflow/error scenarios forsubtract().Current assertions validate a straightforward hour subtraction only. Add one boundary case that crosses date boundaries and one invalid-duration/error case in this same file.
As per coding guidelines, “tests should cover happy paths, edge cases, and error cases — all in the same file for each method.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Temporal/ZonedDateTime/prototype/subtract.js` around lines 9 - 15, Add two tests for Temporal.ZonedDateTime.prototype.subtract in this file: (1) an edge-case that crosses a date boundary — create a ZonedDateTime near midnight (e.g., hour 0 and minute 30), call zdt.subtract({ hours: 1 }) and assert the resulting day and hour reflect the previous calendar day (use the same zdt variable and check .day and .hour or equivalent properties); (2) an error-case for invalid duration input — call zdt.subtract with an invalid duration like { hours: "foo" } and assert it throws (use expect(() => zdt.subtract(...)).toThrow() with the appropriate error). Ensure both tests live alongside the existing "subtract duration" test and reference the same ZonedDateTime/ subtract usage.tests/built-ins/Temporal/Duration/prototype/round.js (1)
9-27: Nice baseline set; add one explicit invalid-options case.These tests cover multiple happy paths well. Please add at least one invalid-options assertion (e.g., unsupported
roundingModeor unit) and a boundary tie case to lock behavior.As per coding guidelines, “tests should cover happy paths, edge cases, and error cases — all in the same file for each method.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Temporal/Duration/prototype/round.js` around lines 9 - 27, Add two tests for Temporal.Duration.prototype.round: one invalid-options case and one boundary tie case. For the invalid case call Duration.prototype.round on a Duration (e.g., new Temporal.Duration(...)) with an unsupported roundingMode or unit (e.g., roundingMode: "invalid" or smallestUnit: "lightyear") and assert it throws the appropriate error (expect(...).toThrow(RangeError/TypeError as applicable). For the tie/boundary case create a duration that ties exactly on the midpoint for hours (e.g., 0 hours 30 minutes) and call round({ smallestUnit: "hour", roundingMode: "halfExpand" }) and assert the resulting hours value matches the expected tie-breaking behavior (1 for halfExpand); add clear test names like "round invalid options" and "round tie boundary" to match existing test style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/built-ins/Temporal/Instant/constructor.js`:
- Around line 2-12: The suite title "Temporal.Instant constructor" is misleading
because the test exercises Temporal.Instant.fromEpochMilliseconds and the
epochNanoseconds getter; update the suite to match the API under test or add a
real constructor test: either rename the describe.runIf title to something like
"Temporal.Instant.fromEpochMilliseconds / epochNanoseconds" and leave the
existing test (referencing Temporal.Instant.fromEpochMilliseconds and
instant.epochNanoseconds), or add a new test that constructs an Instant via the
constructor (if supported) and asserts its epochNanoseconds, keeping test names
and the describe.runIf title consistent with the symbols under test.
In `@units/Goccia.Builtins.Temporal.pas`:
- Around line 997-1021: The code currently lets an absent or malformed month
fall through as M = 0 and defers failure to
TGocciaTemporalPlainMonthDayValue.Create; update the parsing logic around
Obj.GetProperty('monthCode') / Obj.GetProperty('month') so that if neither
property is present/defined or the parsed month value (M) is not a valid month
(e.g., not in 1..12) you call ThrowTypeError('Temporal.PlainMonthDay.from
requires month property') immediately (similar to the existing day check),
ensuring TGocciaObjectValue handling, monthCode parsing and numeric month
truncation are validated before calling
TGocciaTemporalPlainMonthDayValue.Create.
- Around line 1157-1175: CoerceZDT currently only accepts
TGocciaTemporalZonedDateTimeValue instances; change it to also accept string
inputs by reusing the same string coercion/parsing path used by the
ZonedDateTime.from surface: if AArg is TGocciaTemporalZonedDateTimeValue cast to
TGocciaTemporalZonedDateTimeValue, else if AArg is a string value call the
existing/from-parsing function used by ZonedDateTime.from to produce a
TGocciaTemporalZonedDateTimeValue, otherwise call ThrowTypeError; update
ZonedDateTimeCompare to use this revised CoerceZDT so ISO strings compare
correctly.
- Around line 497-519: The code only constrains Day > MaxDay but doesn't clamp
month or low day values when Overflow is set to constrain; update the
Temporal.PlainDate.from flow so after reading Y, Mo, Dy (via Obj.GetProperty and
Trunc(V.ToNumberLiteral.Value)) you first clamp Mo into 1..12 (if Overflow <>
toReject) then recompute MaxDay := DaysInMonth(Y, clampedMo) and clamp Dy into
1..MaxDay (also handling Dy < 1) before calling
TGocciaTemporalPlainDateValue.Create; keep existing ThrowRangeError behavior
when Overflow = toReject and use ThrowTypeError for missing props as before.
In `@units/Goccia.Values.TemporalDuration.pas`:
- Around line 564-567: SmallestUnit may be tuWeek and UnitToNanoseconds rejects
calendar units, so handle tuWeek before calling UnitToNanoseconds: if
SmallestUnit = tuWeek compute Divisor as 7 * UnitToNanoseconds(tuDay) *
Increment (or otherwise convert weeks to days/nanoseconds) and then call
RoundWithMode; for all other non-calendar units continue to call
UnitToNanoseconds(SmallestUnit) * Increment. Refer to SmallestUnit, tuWeek,
UnitToNanoseconds and RoundWithMode to locate and update the branch.
In `@units/Goccia.Values.TemporalPlainDateTime.pas`:
- Around line 790-815: GetUtcOffsetSeconds currently falls back to 0 when zone
data is missing, so update the code to reject unknown zone IDs: change
GetUtcOffsetSeconds to either raise an exception on unknown/failed zone lookup
or provide a boolean/sentinel indicating failure, then in the
PlainDateTime->toZonedDateTime code (the block constructing EpochMs and calling
GetUtcOffsetSeconds) check that lookup result and if it indicates failure call
ThrowTypeError('PlainDateTime.prototype.toZonedDateTime requires a valid
timeZone') instead of using offset 0; reference the GetUtcOffsetSeconds function
and the TGocciaTemporalZonedDateTimeValue creation site so the invalid ID is
rejected before creating the result.
In `@units/Goccia.Values.TemporalPlainMonthDay.pas`:
- Around line 227-239: The current branch only reads monthCode and never honors
a numeric month property, so when callers pass a property-bag like { month: 8 }
NewMonth remains MD.FMonth; fix by reading and applying the numeric 'month'
property before/alongside the existing monthCode handling: call
Obj.GetProperty('month'), check Assigned(V) and not
TGocciaUndefinedLiteralValue, extract/validate the integer (using TryStrToInt or
the existing month-field helper) and assign NewMonth accordingly, then keep the
monthCode logic to validate/override per the specification; update the code
around NewMonth, Obj.GetProperty('monthCode'), and the subsequent
TGocciaTemporalPlainMonthDayValue.Create(...) use.
- Around line 94-110: Coerce the PlainMonthDay object to accept numeric "month"
like the constructor: when AValue is TGocciaObjectValue, if
Obj.GetProperty('monthCode') is nil or undefined then try
Obj.GetProperty('month') and, if present and numeric, use its integer value as
MonthPart (validate range as needed) instead of throwing; otherwise fall back to
parsing monthCode as currently done; update the code paths around
VMonthCode/VDay, MonthPart and the call to
TGocciaTemporalPlainMonthDayValue.Create to use the derived MonthPart from
either monthCode or month and keep ThrowTypeError for missing/invalid inputs.
In `@units/Goccia.Values.TemporalZonedDateTime.pas`:
- Around line 267-276: The constructor TGocciaTemporalZonedDateTimeValue.Create
currently accepts any non-empty ATimeZone and stores bogus zones; before
assigning FTimeZone call a timezone-validation helper (e.g. a new
IsValidTimeZoneId or by attempting to load the zone via the existing TimeZone
loader) and throw a TypeError if the identifier cannot be resolved; reference
TGocciaTemporalZonedDateTimeValue.Create and the TimeZone
loader/GetUtcOffsetSeconds logic to implement the check so unknown IDs are
rejected at construction (or add a small helper in the TimeZone unit to detect
load failures and use it here).
- Around line 939-959: The day-branch currently uses a fixed NANOSECONDS_PER_DAY
which breaks across DST; instead compute the actual length of the local day in
nanoseconds and use that as the divisor. Specifically: compute StartEpochMs :=
LocalToEpochMs(LYear,LMonth,LDay,0,0,0,0,Zdt.FTimeZone), compute NextDate :=
AddDaysToDate(LYear,LMonth,LDay,1) and NextEpochMs :=
LocalToEpochMs(NextDate.Year,NextDate.Month,NextDate.Day,0,0,0,0,Zdt.FTimeZone),
set DayNs := Int64(NextEpochMs - StartEpochMs) * NANOSECONDS_PER_MILLISECOND,
set Divisor := DayNs * Increment, call Rounded := RoundWithMode(TotalNs,
Divisor, Mode), compute ExtraDays := Rounded div DayNs and then use
AddDaysToDate/LYear/LMonth/LDay/ExtraDays (or set DateRec to LYear/LMonth/LDay
when ExtraDays=0) and use StartEpochMs (or adjusted epoch) when creating
TGocciaTemporalZonedDateTimeValue; you can also use GetHoursInDay as a helper to
validate the computed day length if desired.
---
Nitpick comments:
In `@tests/built-ins/Temporal/Duration/prototype/round.js`:
- Around line 9-27: Add two tests for Temporal.Duration.prototype.round: one
invalid-options case and one boundary tie case. For the invalid case call
Duration.prototype.round on a Duration (e.g., new Temporal.Duration(...)) with
an unsupported roundingMode or unit (e.g., roundingMode: "invalid" or
smallestUnit: "lightyear") and assert it throws the appropriate error
(expect(...).toThrow(RangeError/TypeError as applicable). For the tie/boundary
case create a duration that ties exactly on the midpoint for hours (e.g., 0
hours 30 minutes) and call round({ smallestUnit: "hour", roundingMode:
"halfExpand" }) and assert the resulting hours value matches the expected
tie-breaking behavior (1 for halfExpand); add clear test names like "round
invalid options" and "round tie boundary" to match existing test style.
In `@tests/built-ins/Temporal/Duration/prototype/total.js`:
- Around line 9-25: Add a test that calls Temporal.Duration.prototype.total
using the options-bag form (e.g., d.total({ unit: "minutes" })) to ensure option
parsing is exercised; update the tests in
tests/built-ins/Temporal/Duration/prototype/total.js to include at least one
assertion using the object overload (for example mirroring an existing
string-based assertion like expect(d.total({ unit: "minutes" })).toBe(90)) and
optionally add a corresponding RangeError assertion that uses the options object
(e.g., expect(() => withYears.total({ unit: "days" })).toThrow(RangeError)).
In `@tests/built-ins/Temporal/Instant/constructor.js`:
- Around line 9-12: Add edge and error-case tests to the existing
"epochNanoseconds getter" test file by extending or adding tests around
Temporal.Instant.fromEpochMilliseconds and the epochNanoseconds property:
include at least one invalid-input case (e.g., pass a non-numeric value like a
string or NaN and assert it throws), and a boundary case (e.g., very large or
negative epoch milliseconds to assert correct epochNanoseconds or an
out-of-range error). Locate the existing test block using the test name
"epochNanoseconds getter" and the Temporal.Instant.fromEpochMilliseconds call,
and add assertions that expect throws for invalid inputs and precise conversions
for boundary values to ensure full method coverage.
In `@tests/built-ins/Temporal/Instant/from.js`:
- Around line 9-13: The Instant.from test currently only asserts epoch
milliseconds equality for the Instant-input path; add an identity assertion to
ensure Instant.from produces a distinct object. In the "from() with Instant"
test that creates i1 via Temporal.Instant.fromEpochMilliseconds and i2 via
Temporal.Instant.from(i1), insert an assertion expect(i2).not.toBe(i1) (in
addition to the existing epochMilliseconds check) to validate the copy-not-alias
behavior of Instant.from.
- Around line 15-18: Add negative test cases for Temporal.Instant.from to cover
invalid inputs: add tests that call Temporal.Instant.from with a malformed ISO
string (e.g., "not-a-date") and with unsupported types (e.g., an object, null,
or a non-string/non-temporal value) and assert that each call throws a
RangeError/TypeError as appropriate; locate calls to Temporal.Instant.from in
this file and create new tests named like "from() with malformed string" and
"from() with unsupported type" that use expect(() =>
Temporal.Instant.from(...)).toThrow() to verify error behavior.
In `@tests/built-ins/Temporal/Now/zonedDateTimeISO.js`:
- Around line 9-15: Add two new tests to
tests/built-ins/Temporal/Now/zonedDateTimeISO.js: one that calls
Temporal.Now.zonedDateTimeISO("America/New_York") (or another valid IANA zone)
and asserts the returned object is a ZonedDateTime with a non-empty timeZoneId
equal to the provided zone or that its timeZoneId includes the expected zone
identifier and epochMilliseconds > 0; and another that calls
Temporal.Now.zonedDateTimeISO with an invalid argument (e.g., a number or
"Invalid/Zone") and asserts that it throws the appropriate error (RangeError or
TypeError) using expect(() => Temporal.Now.zonedDateTimeISO(badArg)).toThrow();
reference the Temporal.Now.zonedDateTimeISO call site in the file when adding
these tests.
In `@tests/built-ins/Temporal/PlainDate/compare.js`:
- Around line 9-15: Add edge-case assertions to the existing compare() test in
tests/built-ins/Temporal/PlainDate/compare.js: call Temporal.PlainDate.compare
with ISO date strings (e.g., "2024-01-01" and "2024-06-01") and assert the same
-1/1/0 results as the PlainDate instances, and add assertions that invalid
inputs (e.g., null, an object, or a non-ISO string) throw a TypeError when
passed to Temporal.PlainDate.compare; update the single "compare()" test to
include these ISO-string happy-path checks and the invalid-input throws so all
cases are co-located with the existing Temporal.PlainDate.compare(d1, d2)
assertions.
In `@tests/built-ins/Temporal/PlainDate/prototype/add.js`:
- Around line 9-21: Add one error-path test for Temporal.PlainDate.prototype.add
that asserts a TypeError is thrown for an invalid duration-like input: create a
PlainDate (e.g. new Temporal.PlainDate(2024,1,1)) and call its add method with
an invalid value (e.g. a plain string or an object with wrong-typed fields),
then use expect(() => d.add(...)).toThrow(TypeError) — name the test "add() with
invalid duration" to match existing tests.
In `@tests/built-ins/Temporal/PlainDateTime/prototype/valueOf.js`:
- Line 11: The test currently only asserts that dt.valueOf() throws a TypeError;
update the assertion to also verify the exact error message thrown by valueOf so
the test locks to the implementation message from
units/Goccia.Values.TemporalPlainDateTime.pas (the message emitted around lines
742–744). Replace or extend the current expect(() =>
dt.valueOf()).toThrow(TypeError) with a message-aware assertion (e.g., the test
harness's toThrow/toThrowError variant that accepts a message or regex) and use
the exact string used in the implementation to ensure strict matching.
In `@tests/built-ins/Temporal/PlainYearMonth/prototype/toPlainDate.js`:
- Around line 1-16: Add an edge-case test in the same file that verifies
Temporal.PlainYearMonth.prototype.toPlainDate behavior when the provided day
overflows the month: create a PlainYearMonth (e.g., ym = new
Temporal.PlainYearMonth(2024, 2)) and assert that calling ym.toPlainDate({ day:
31 }) throws (use expect(() => ym.toPlainDate({ day: 31 })).toThrow() or the
appropriate RangeError expectation) so the test documents whether toPlainDate
rejects out-of-range days.
In `@tests/built-ins/Temporal/ZonedDateTime/prototype/add.js`:
- Around line 9-15: Add two new tests in the same file alongside the existing
"add duration" test: one boundary test that uses a ZonedDateTime near the end of
a day/month (e.g., create zdt at 23:30 on Jan 31 or just before midnight) and
call zdt.add with a duration that crosses into the next day/month, then assert
the expected hour/day/month values; and one error test that calls zdt.add with
an invalid duration (e.g., a non-object, NaN, or a disallowed property) and
asserts it throws (use expect(() => zdt.add(...)).toThrow()). Reference the
existing variable and method names (zdt, ZonedDateTime, zdt.add) so the new
tests match style and placement with the current "add duration" test.
In `@tests/built-ins/Temporal/ZonedDateTime/prototype/startOfDay.js`:
- Around line 8-19: Add a non‑UTC boundary test for
Temporal.ZonedDateTime.prototype.startOfDay in the same file: create a second
test (e.g., "startOfDay DST/boundary") that constructs a ZonedDateTime (zdt) in
a DST‑observing zone such as "America/New_York" at an instant that lies on or
around a DST transition, call zdt.startOfDay() to get sod, and assert the
expected hour/minute/second/year/month/day for that local start of day so the
edge behaviour (clock jump/back) is covered alongside the existing UTC happy
path.
In `@tests/built-ins/Temporal/ZonedDateTime/prototype/subtract.js`:
- Around line 9-15: Add two tests for Temporal.ZonedDateTime.prototype.subtract
in this file: (1) an edge-case that crosses a date boundary — create a
ZonedDateTime near midnight (e.g., hour 0 and minute 30), call zdt.subtract({
hours: 1 }) and assert the resulting day and hour reflect the previous calendar
day (use the same zdt variable and check .day and .hour or equivalent
properties); (2) an error-case for invalid duration input — call zdt.subtract
with an invalid duration like { hours: "foo" } and assert it throws (use
expect(() => zdt.subtract(...)).toThrow() with the appropriate error). Ensure
both tests live alongside the existing "subtract duration" test and reference
the same ZonedDateTime/ subtract usage.
In `@tests/built-ins/Temporal/ZonedDateTime/prototype/withPlainTime.js`:
- Around line 1-20: Add two edge-case tests to
tests/built-ins/Temporal/ZonedDateTime/prototype/withPlainTime.js: one that
calls ZonedDateTime.prototype.withPlainTime with undefined (e.g.,
zdt.withPlainTime(undefined)) and asserts the time resets to midnight
(hour/minute/second = 0) while date remains unchanged, and another that passes a
time string (e.g., "00:00" or "12:34:56") to withPlainTime and asserts the
resulting hour/minute/second reflect that parsed time while year/month/day
remain unchanged; locate the existing test using the zdt variable and function
under test (withPlainTime / Temporal.ZonedDateTime) and follow the same
describe.runIf(isTemporal) / test structure and assertion style as the existing
cases.
In `@units/Goccia.Temporal.TimeZone.pas`:
- Around line 126-133: Extract the magic offset 20 into a named constant (e.g.
TZIFHeaderOffset or HeaderStartOffset) declared in the implementation section
and replace the literal in the line "Offset := 20;" with that constant; update
any related comments to reference the constant and leave the subsequent
ReadBigEndianInt32 calls (IsUtCnt, IsStdCnt, LeapCnt, TimeCnt, TypeCnt, CharCnt)
unchanged so the intent that the header starts after magic+version+reserved
bytes is clear.
- Around line 288-307: Replace manual Stream.Free calls with a try..finally to
guarantee Stream is freed; create the TFileStream (TFileStream.Create) before a
try, perform FileSize checks, SetLength(Buffer) and Stream.ReadBuffer(Buffer[0],
FileSize) inside the try, then in the finally call Stream.Free or
FreeAndNil(Stream) if assigned. Remove the extra Stream.Free calls inside the if
and except blocks and ensure the function still calls ParseTZifData(ATimeZone,
Buffer, FileSize, AData) after reading; reference symbols: TFileStream.Create,
Stream.ReadBuffer, ParseTZifData, Stream.Free/FreeAndNil.
🪄 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: CHILL
Plan: Pro
Run ID: d06e50a3-7789-4d93-972d-6fa8c1b86911
📒 Files selected for processing (151)
tests/built-ins/Temporal/Duration/compare.jstests/built-ins/Temporal/Duration/constructor.jstests/built-ins/Temporal/Duration/from.jstests/built-ins/Temporal/Duration/prototype/abs.jstests/built-ins/Temporal/Duration/prototype/add.jstests/built-ins/Temporal/Duration/prototype/blank.jstests/built-ins/Temporal/Duration/prototype/negated.jstests/built-ins/Temporal/Duration/prototype/round.jstests/built-ins/Temporal/Duration/prototype/sign.jstests/built-ins/Temporal/Duration/prototype/subtract.jstests/built-ins/Temporal/Duration/prototype/toJSON.jstests/built-ins/Temporal/Duration/prototype/toString.jstests/built-ins/Temporal/Duration/prototype/toStringTag.jstests/built-ins/Temporal/Duration/prototype/total.jstests/built-ins/Temporal/Duration/prototype/valueOf.jstests/built-ins/Temporal/Duration/prototype/with.jstests/built-ins/Temporal/Instant/compare.jstests/built-ins/Temporal/Instant/constructor.jstests/built-ins/Temporal/Instant/from.jstests/built-ins/Temporal/Instant/fromEpochMilliseconds.jstests/built-ins/Temporal/Instant/fromEpochNanoseconds.jstests/built-ins/Temporal/Instant/prototype/add.jstests/built-ins/Temporal/Instant/prototype/equals.jstests/built-ins/Temporal/Instant/prototype/round.jstests/built-ins/Temporal/Instant/prototype/since.jstests/built-ins/Temporal/Instant/prototype/subtract.jstests/built-ins/Temporal/Instant/prototype/toJSON.jstests/built-ins/Temporal/Instant/prototype/toString.jstests/built-ins/Temporal/Instant/prototype/toStringTag.jstests/built-ins/Temporal/Instant/prototype/until.jstests/built-ins/Temporal/Instant/prototype/valueOf.jstests/built-ins/Temporal/Now/instant.jstests/built-ins/Temporal/Now/plainDateISO.jstests/built-ins/Temporal/Now/plainDateTimeISO.jstests/built-ins/Temporal/Now/plainTimeISO.jstests/built-ins/Temporal/Now/timeZoneId.jstests/built-ins/Temporal/Now/zonedDateTimeISO.jstests/built-ins/Temporal/PlainDate/compare.jstests/built-ins/Temporal/PlainDate/constructor.jstests/built-ins/Temporal/PlainDate/from.jstests/built-ins/Temporal/PlainDate/prototype/add.jstests/built-ins/Temporal/PlainDate/prototype/calendarId.jstests/built-ins/Temporal/PlainDate/prototype/dayOfWeek.jstests/built-ins/Temporal/PlainDate/prototype/equals.jstests/built-ins/Temporal/PlainDate/prototype/since.jstests/built-ins/Temporal/PlainDate/prototype/subtract.jstests/built-ins/Temporal/PlainDate/prototype/toJSON.jstests/built-ins/Temporal/PlainDate/prototype/toPlainDateTime.jstests/built-ins/Temporal/PlainDate/prototype/toPlainMonthDay.jstests/built-ins/Temporal/PlainDate/prototype/toPlainYearMonth.jstests/built-ins/Temporal/PlainDate/prototype/toString.jstests/built-ins/Temporal/PlainDate/prototype/toStringTag.jstests/built-ins/Temporal/PlainDate/prototype/toZonedDateTime.jstests/built-ins/Temporal/PlainDate/prototype/until.jstests/built-ins/Temporal/PlainDate/prototype/valueOf.jstests/built-ins/Temporal/PlainDate/prototype/with.jstests/built-ins/Temporal/PlainDateTime/compare.jstests/built-ins/Temporal/PlainDateTime/constructor.jstests/built-ins/Temporal/PlainDateTime/from.jstests/built-ins/Temporal/PlainDateTime/prototype/add.jstests/built-ins/Temporal/PlainDateTime/prototype/equals.jstests/built-ins/Temporal/PlainDateTime/prototype/round.jstests/built-ins/Temporal/PlainDateTime/prototype/since.jstests/built-ins/Temporal/PlainDateTime/prototype/subtract.jstests/built-ins/Temporal/PlainDateTime/prototype/toJSON.jstests/built-ins/Temporal/PlainDateTime/prototype/toPlainDate.jstests/built-ins/Temporal/PlainDateTime/prototype/toPlainMonthDay.jstests/built-ins/Temporal/PlainDateTime/prototype/toPlainTime.jstests/built-ins/Temporal/PlainDateTime/prototype/toPlainYearMonth.jstests/built-ins/Temporal/PlainDateTime/prototype/toString.jstests/built-ins/Temporal/PlainDateTime/prototype/toStringTag.jstests/built-ins/Temporal/PlainDateTime/prototype/toZonedDateTime.jstests/built-ins/Temporal/PlainDateTime/prototype/until.jstests/built-ins/Temporal/PlainDateTime/prototype/valueOf.jstests/built-ins/Temporal/PlainDateTime/prototype/with.jstests/built-ins/Temporal/PlainDateTime/prototype/withPlainTime.jstests/built-ins/Temporal/PlainMonthDay/compare.jstests/built-ins/Temporal/PlainMonthDay/constructor.jstests/built-ins/Temporal/PlainMonthDay/from.jstests/built-ins/Temporal/PlainMonthDay/prototype/equals.jstests/built-ins/Temporal/PlainMonthDay/prototype/toJSON.jstests/built-ins/Temporal/PlainMonthDay/prototype/toPlainDate.jstests/built-ins/Temporal/PlainMonthDay/prototype/toString.jstests/built-ins/Temporal/PlainMonthDay/prototype/toStringTag.jstests/built-ins/Temporal/PlainMonthDay/prototype/valueOf.jstests/built-ins/Temporal/PlainTime/compare.jstests/built-ins/Temporal/PlainTime/constructor.jstests/built-ins/Temporal/PlainTime/from.jstests/built-ins/Temporal/PlainTime/prototype/add.jstests/built-ins/Temporal/PlainTime/prototype/equals.jstests/built-ins/Temporal/PlainTime/prototype/round.jstests/built-ins/Temporal/PlainTime/prototype/since.jstests/built-ins/Temporal/PlainTime/prototype/subtract.jstests/built-ins/Temporal/PlainTime/prototype/toJSON.jstests/built-ins/Temporal/PlainTime/prototype/toPlainDateTime.jstests/built-ins/Temporal/PlainTime/prototype/toString.jstests/built-ins/Temporal/PlainTime/prototype/toStringTag.jstests/built-ins/Temporal/PlainTime/prototype/until.jstests/built-ins/Temporal/PlainTime/prototype/valueOf.jstests/built-ins/Temporal/PlainTime/prototype/with.jstests/built-ins/Temporal/PlainYearMonth/compare.jstests/built-ins/Temporal/PlainYearMonth/constructor.jstests/built-ins/Temporal/PlainYearMonth/from.jstests/built-ins/Temporal/PlainYearMonth/prototype/add.jstests/built-ins/Temporal/PlainYearMonth/prototype/equals.jstests/built-ins/Temporal/PlainYearMonth/prototype/since.jstests/built-ins/Temporal/PlainYearMonth/prototype/subtract.jstests/built-ins/Temporal/PlainYearMonth/prototype/toJSON.jstests/built-ins/Temporal/PlainYearMonth/prototype/toPlainDate.jstests/built-ins/Temporal/PlainYearMonth/prototype/toString.jstests/built-ins/Temporal/PlainYearMonth/prototype/toStringTag.jstests/built-ins/Temporal/PlainYearMonth/prototype/until.jstests/built-ins/Temporal/PlainYearMonth/prototype/valueOf.jstests/built-ins/Temporal/PlainYearMonth/prototype/with.jstests/built-ins/Temporal/ZonedDateTime/compare.jstests/built-ins/Temporal/ZonedDateTime/constructor.jstests/built-ins/Temporal/ZonedDateTime/from.jstests/built-ins/Temporal/ZonedDateTime/prototype/add.jstests/built-ins/Temporal/ZonedDateTime/prototype/calendarGetters.jstests/built-ins/Temporal/ZonedDateTime/prototype/equals.jstests/built-ins/Temporal/ZonedDateTime/prototype/hoursInDay.jstests/built-ins/Temporal/ZonedDateTime/prototype/round.jstests/built-ins/Temporal/ZonedDateTime/prototype/startOfDay.jstests/built-ins/Temporal/ZonedDateTime/prototype/subtract.jstests/built-ins/Temporal/ZonedDateTime/prototype/toInstant.jstests/built-ins/Temporal/ZonedDateTime/prototype/toJSON.jstests/built-ins/Temporal/ZonedDateTime/prototype/toPlainDate.jstests/built-ins/Temporal/ZonedDateTime/prototype/toPlainDateTime.jstests/built-ins/Temporal/ZonedDateTime/prototype/toPlainTime.jstests/built-ins/Temporal/ZonedDateTime/prototype/toString.jstests/built-ins/Temporal/ZonedDateTime/prototype/toStringTag.jstests/built-ins/Temporal/ZonedDateTime/prototype/valueOf.jstests/built-ins/Temporal/ZonedDateTime/prototype/with.jstests/built-ins/Temporal/ZonedDateTime/prototype/withPlainTime.jstests/built-ins/Temporal/ZonedDateTime/prototype/withTimeZone.jstests/built-ins/Temporal/duration.jstests/built-ins/Temporal/instant.jstests/built-ins/Temporal/now.jstests/built-ins/Temporal/plain-date-time.jstests/built-ins/Temporal/plain-date.jstests/built-ins/Temporal/plain-time.jstests/built-ins/Temporal/toStringTag.jsunits/Goccia.Builtins.Temporal.pasunits/Goccia.Temporal.Options.pasunits/Goccia.Temporal.TimeZone.pasunits/Goccia.Temporal.Utils.pasunits/Goccia.Values.TemporalDuration.pasunits/Goccia.Values.TemporalPlainDate.pasunits/Goccia.Values.TemporalPlainDateTime.pasunits/Goccia.Values.TemporalPlainMonthDay.pasunits/Goccia.Values.TemporalZonedDateTime.pas
💤 Files with no reviewable changes (6)
- tests/built-ins/Temporal/plain-time.js
- tests/built-ins/Temporal/plain-date.js
- tests/built-ins/Temporal/plain-date-time.js
- tests/built-ins/Temporal/duration.js
- tests/built-ins/Temporal/instant.js
- tests/built-ins/Temporal/now.js
✅ Files skipped from review due to trivial changes (107)
- tests/built-ins/Temporal/PlainTime/prototype/with.js
- tests/built-ins/Temporal/Duration/compare.js
- tests/built-ins/Temporal/Duration/prototype/abs.js
- tests/built-ins/Temporal/Duration/prototype/add.js
- tests/built-ins/Temporal/Duration/prototype/blank.js
- tests/built-ins/Temporal/Duration/prototype/negated.js
- tests/built-ins/Temporal/Duration/prototype/sign.js
- tests/built-ins/Temporal/Duration/prototype/subtract.js
- tests/built-ins/Temporal/Duration/prototype/toJSON.js
- tests/built-ins/Temporal/Duration/prototype/toString.js
- tests/built-ins/Temporal/Duration/prototype/valueOf.js
- tests/built-ins/Temporal/Duration/prototype/with.js
- tests/built-ins/Temporal/Instant/fromEpochNanoseconds.js
- tests/built-ins/Temporal/Instant/prototype/equals.js
- tests/built-ins/Temporal/Instant/prototype/subtract.js
- tests/built-ins/Temporal/Instant/prototype/since.js
- tests/built-ins/Temporal/Instant/prototype/toString.js
- tests/built-ins/Temporal/Instant/prototype/toStringTag.js
- tests/built-ins/Temporal/Instant/prototype/until.js
- tests/built-ins/Temporal/Instant/prototype/valueOf.js
- tests/built-ins/Temporal/Now/instant.js
- tests/built-ins/Temporal/Now/plainDateTimeISO.js
- tests/built-ins/Temporal/Now/plainTimeISO.js
- tests/built-ins/Temporal/Now/timeZoneId.js
- tests/built-ins/Temporal/PlainDate/constructor.js
- tests/built-ins/Temporal/PlainDate/prototype/calendarId.js
- tests/built-ins/Temporal/PlainDate/prototype/equals.js
- tests/built-ins/Temporal/PlainDate/prototype/since.js
- tests/built-ins/Temporal/PlainDate/prototype/subtract.js
- tests/built-ins/Temporal/PlainDate/prototype/toJSON.js
- tests/built-ins/Temporal/PlainDate/prototype/toPlainDateTime.js
- tests/built-ins/Temporal/PlainDate/prototype/toPlainMonthDay.js
- tests/built-ins/Temporal/PlainDate/prototype/toPlainYearMonth.js
- tests/built-ins/Temporal/PlainDate/prototype/toString.js
- tests/built-ins/Temporal/PlainDate/prototype/toStringTag.js
- tests/built-ins/Temporal/PlainDate/prototype/toZonedDateTime.js
- tests/built-ins/Temporal/PlainDate/prototype/until.js
- tests/built-ins/Temporal/PlainDate/prototype/valueOf.js
- tests/built-ins/Temporal/PlainDate/prototype/with.js
- tests/built-ins/Temporal/PlainDateTime/compare.js
- tests/built-ins/Temporal/PlainDateTime/from.js
- tests/built-ins/Temporal/PlainDateTime/prototype/add.js
- tests/built-ins/Temporal/PlainDateTime/prototype/equals.js
- tests/built-ins/Temporal/PlainDateTime/prototype/round.js
- tests/built-ins/Temporal/PlainDateTime/prototype/since.js
- tests/built-ins/Temporal/PlainDateTime/prototype/subtract.js
- tests/built-ins/Temporal/PlainDateTime/prototype/toJSON.js
- tests/built-ins/Temporal/PlainDateTime/prototype/toPlainDate.js
- tests/built-ins/Temporal/PlainDateTime/prototype/toPlainMonthDay.js
- tests/built-ins/Temporal/PlainDateTime/prototype/toPlainYearMonth.js
- tests/built-ins/Temporal/PlainDateTime/prototype/toString.js
- tests/built-ins/Temporal/PlainDateTime/prototype/toZonedDateTime.js
- tests/built-ins/Temporal/PlainDateTime/prototype/until.js
- tests/built-ins/Temporal/PlainDateTime/prototype/with.js
- tests/built-ins/Temporal/PlainDateTime/prototype/withPlainTime.js
- tests/built-ins/Temporal/PlainMonthDay/compare.js
- tests/built-ins/Temporal/PlainMonthDay/constructor.js
- tests/built-ins/Temporal/PlainMonthDay/from.js
- tests/built-ins/Temporal/PlainMonthDay/prototype/equals.js
- tests/built-ins/Temporal/PlainMonthDay/prototype/toJSON.js
- tests/built-ins/Temporal/PlainMonthDay/prototype/toPlainDate.js
- tests/built-ins/Temporal/PlainMonthDay/prototype/toString.js
- tests/built-ins/Temporal/PlainMonthDay/prototype/toStringTag.js
- tests/built-ins/Temporal/PlainTime/compare.js
- tests/built-ins/Temporal/PlainTime/from.js
- tests/built-ins/Temporal/PlainTime/constructor.js
- tests/built-ins/Temporal/PlainTime/prototype/add.js
- tests/built-ins/Temporal/PlainTime/prototype/equals.js
- tests/built-ins/Temporal/PlainTime/prototype/since.js
- tests/built-ins/Temporal/PlainTime/prototype/subtract.js
- tests/built-ins/Temporal/PlainTime/prototype/toJSON.js
- tests/built-ins/Temporal/PlainTime/prototype/toPlainDateTime.js
- tests/built-ins/Temporal/PlainTime/prototype/toString.js
- tests/built-ins/Temporal/PlainTime/prototype/toStringTag.js
- tests/built-ins/Temporal/PlainYearMonth/prototype/add.js
- tests/built-ins/Temporal/PlainYearMonth/from.js
- tests/built-ins/Temporal/PlainYearMonth/prototype/subtract.js
- tests/built-ins/Temporal/PlainYearMonth/prototype/since.js
- tests/built-ins/Temporal/PlainYearMonth/prototype/toJSON.js
- tests/built-ins/Temporal/PlainYearMonth/prototype/equals.js
- tests/built-ins/Temporal/PlainYearMonth/prototype/toStringTag.js
- tests/built-ins/Temporal/PlainYearMonth/prototype/toString.js
- tests/built-ins/Temporal/PlainYearMonth/prototype/until.js
- tests/built-ins/Temporal/ZonedDateTime/compare.js
- tests/built-ins/Temporal/PlainYearMonth/prototype/valueOf.js
- tests/built-ins/Temporal/ZonedDateTime/prototype/equals.js
- tests/built-ins/Temporal/PlainYearMonth/prototype/with.js
- tests/built-ins/Temporal/ZonedDateTime/prototype/calendarGetters.js
- tests/built-ins/Temporal/ZonedDateTime/prototype/toPlainTime.js
- tests/built-ins/Temporal/ZonedDateTime/prototype/round.js
- tests/built-ins/Temporal/ZonedDateTime/prototype/toPlainDateTime.js
- tests/built-ins/Temporal/ZonedDateTime/prototype/toPlainDate.js
- tests/built-ins/Temporal/ZonedDateTime/prototype/hoursInDay.js
- tests/built-ins/Temporal/PlainMonthDay/prototype/valueOf.js
- tests/built-ins/Temporal/ZonedDateTime/from.js
- tests/built-ins/Temporal/Now/plainDateISO.js
- tests/built-ins/Temporal/PlainDate/prototype/dayOfWeek.js
- tests/built-ins/Temporal/PlainDateTime/constructor.js
- tests/built-ins/Temporal/PlainDateTime/prototype/toPlainTime.js
- tests/built-ins/Temporal/ZonedDateTime/prototype/valueOf.js
- tests/built-ins/Temporal/PlainTime/prototype/until.js
- tests/built-ins/Temporal/PlainTime/prototype/round.js
- tests/built-ins/Temporal/PlainYearMonth/compare.js
- tests/built-ins/Temporal/Instant/compare.js
- tests/built-ins/Temporal/ZonedDateTime/prototype/withTimeZone.js
- tests/built-ins/Temporal/ZonedDateTime/prototype/toStringTag.js
- tests/built-ins/Temporal/Instant/fromEpochMilliseconds.js
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/built-ins/Temporal/toStringTag.js
- units/Goccia.Values.TemporalPlainDate.pas
Address 11 new CodeRabbit findings: - Fix PlainDate.from overflow constrain: clamp month to 1..12 and day >= 1 - Fix PlainMonthDay.from: throw TypeError on missing/invalid month - Fix CoercePlainMonthDay to accept numeric month (not just monthCode) - Fix PlainMonthDay.with to accept numeric month property - Fix ZonedDateTime.compare to accept ISO strings (same as from) - Fix Duration.round: handle smallestUnit "week" before UnitToNanoseconds - Add timezone validation in ZonedDateTime constructor via IsValidTimeZone - Add timezone validation in PlainDateTime.toZonedDateTime - Fix ZonedDateTime.round day branch to use DST-aware day length - Fix Instant/constructor.js test title mismatch (merged into fromEpochMilliseconds.js) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
units/Goccia.Values.TemporalDuration.pas (1)
538-550:⚠️ Potential issue | 🟠 MajorNormalize and validate
largestUnitbefore decomposition.This block still mishandles two cases: the default scan skips
FWeeks, soTemporal.Duration.from({ weeks: 1 }).round({ smallestUnit: 'day' })collapses to days, and explicit contradictory options like{ largestUnit: 'minute', smallestUnit: 'day' }are accepted because the ordering check only runs whenlargestUnitwas omitted. Both should be fixed before the breakdown step.Suggested fix
if LargestUnit = tuNone then begin // Default largestUnit to the largest defined unit of the duration - if D.FDays <> 0 then LargestUnit := tuDay + if D.FWeeks <> 0 then LargestUnit := tuWeek + else if D.FDays <> 0 then LargestUnit := tuDay else if D.FHours <> 0 then LargestUnit := tuHour else if D.FMinutes <> 0 then LargestUnit := tuMinute else if D.FSeconds <> 0 then LargestUnit := tuSecond else if D.FMilliseconds <> 0 then LargestUnit := tuMillisecond else if D.FMicroseconds <> 0 then LargestUnit := tuMicrosecond else LargestUnit := SmallestUnit; // Ensure largestUnit >= smallestUnit if Ord(LargestUnit) > Ord(SmallestUnit) then LargestUnit := SmallestUnit; end; + if (LargestUnit <> tuNone) and (Ord(LargestUnit) > Ord(SmallestUnit)) then + ThrowRangeError('largestUnit must be greater than or equal to smallestUnit');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.TemporalDuration.pas` around lines 538 - 550, The LargestUnit defaulting/validation block must include weeks in the default scan and always enforce ordering regardless of whether LargestUnit was provided: when LargestUnit = tuNone, include a check for D.FWeeks before days (use tuWeek), and after determining LargestUnit (whether defaulted or explicit) validate that Ord(LargestUnit) <= Ord(SmallestUnit) and if not, set LargestUnit := SmallestUnit; apply this change in the existing block around LargestUnit/SmallestUnit before the decomposition code (symbols: LargestUnit, SmallestUnit, D.FWeeks, tuWeek).
🧹 Nitpick comments (1)
units/Goccia.Values.TemporalPlainMonthDay.pas (1)
188-297: Normalize ECMAScript spec comment format to the repository standard.The current
// TC39 Temporal §...annotations should follow the required// ESYYYY §X.Y.Z SpecMethodName(specParams)format for spec-traceability consistency.As per coding guidelines, "When implementing ECMAScript-specified behavior, annotate each function with a comment referencing the spec section. Use format // ESYYYY §X.Y.Z SpecMethodName(specParams) ..."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.TemporalPlainMonthDay.pas` around lines 188 - 297, Update the spec-comment headers to the repository standard "// ESYYYY §X.Y.Z SpecMethodName(specParams)" for each implementation in this file: GetCalendarId, GetMonthCode, GetDay, MonthDayWith, MonthDayEquals, MonthDayToString, MonthDayToJSON, MonthDayValueOf and MonthDayToPlainDate; replace the existing "// TC39 Temporal §11.3.x ..." lines with the standard format using the appropriate ES year and the spec method names (e.g. the PlainMonthDay.prototype methods) so each function's comment reads "// ESYYYY §11.3.X PlainMonthDay.prototype.<method>(params)".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@units/Goccia.Values.TemporalPlainMonthDay.pas`:
- Around line 101-116: When parsing monthCode in the block that sets MonthPart
from VMonthCode (variables: VMonthCode, MonthCodeStr, MonthPart, ThrowTypeError,
AMethod, Obj.GetProperty), additionally check whether a 'month' property is
present; if both exist compute the numeric month from monthCode and compare it
to Trunc(V.ToNumberLiteral.Value) for the 'month' property and throw a type
error (e.g. via ThrowTypeError with a message mentioning AMethod) when they
differ. Apply the same consistency check in the second parsing location (the
similar block around lines 237-251) so that any mismatched month/monthCode pairs
are rejected rather than ignoring one of them.
- Around line 97-115: The code uses hardcoded property name string literals
(e.g. 'day', 'monthCode', 'month') when calling Obj.GetProperty and in related
checks inside the temporal parsing routine (see variables VDay, VMonthCode, V
and the ThrowTypeError calls); replace these raw strings with the shared
constants from Goccia.Constants.PropertyNames (use the appropriate constant
names for Day, MonthCode, Month) across the function and the other referenced
accessors (lines around the Obj.GetProperty calls and the checks) so all
property accesses use the central constants rather than inline literals.
---
Duplicate comments:
In `@units/Goccia.Values.TemporalDuration.pas`:
- Around line 538-550: The LargestUnit defaulting/validation block must include
weeks in the default scan and always enforce ordering regardless of whether
LargestUnit was provided: when LargestUnit = tuNone, include a check for
D.FWeeks before days (use tuWeek), and after determining LargestUnit (whether
defaulted or explicit) validate that Ord(LargestUnit) <= Ord(SmallestUnit) and
if not, set LargestUnit := SmallestUnit; apply this change in the existing block
around LargestUnit/SmallestUnit before the decomposition code (symbols:
LargestUnit, SmallestUnit, D.FWeeks, tuWeek).
---
Nitpick comments:
In `@units/Goccia.Values.TemporalPlainMonthDay.pas`:
- Around line 188-297: Update the spec-comment headers to the repository
standard "// ESYYYY §X.Y.Z SpecMethodName(specParams)" for each implementation
in this file: GetCalendarId, GetMonthCode, GetDay, MonthDayWith, MonthDayEquals,
MonthDayToString, MonthDayToJSON, MonthDayValueOf and MonthDayToPlainDate;
replace the existing "// TC39 Temporal §11.3.x ..." lines with the standard
format using the appropriate ES year and the spec method names (e.g. the
PlainMonthDay.prototype methods) so each function's comment reads "// ESYYYY
§11.3.X PlainMonthDay.prototype.<method>(params)".
🪄 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: CHILL
Plan: Pro
Run ID: e9e304e5-4b83-4975-b7e3-b871d885ef8c
📒 Files selected for processing (12)
tests/built-ins/Temporal/Duration/prototype/round.jstests/built-ins/Temporal/Instant/fromEpochMilliseconds.jstests/built-ins/Temporal/PlainDate/from.jstests/built-ins/Temporal/PlainMonthDay/from.jstests/built-ins/Temporal/PlainMonthDay/prototype/equals.jstests/built-ins/Temporal/PlainMonthDay/prototype/with.jstests/built-ins/Temporal/ZonedDateTime/compare.jsunits/Goccia.Builtins.Temporal.pasunits/Goccia.Values.TemporalDuration.pasunits/Goccia.Values.TemporalPlainDateTime.pasunits/Goccia.Values.TemporalPlainMonthDay.pasunits/Goccia.Values.TemporalZonedDateTime.pas
✅ Files skipped from review due to trivial changes (5)
- tests/built-ins/Temporal/PlainMonthDay/prototype/with.js
- tests/built-ins/Temporal/Instant/fromEpochMilliseconds.js
- tests/built-ins/Temporal/PlainMonthDay/prototype/equals.js
- tests/built-ins/Temporal/ZonedDateTime/compare.js
- tests/built-ins/Temporal/PlainDate/from.js
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/built-ins/Temporal/PlainMonthDay/from.js
- tests/built-ins/Temporal/Duration/prototype/round.js
- units/Goccia.Values.TemporalZonedDateTime.pas
- units/Goccia.Builtins.Temporal.pas
…y in PlainMonthDay Add Temporal property constants (PROP_YEAR, PROP_MONTH, PROP_MONTH_CODE, PROP_DAY, PROP_CALENDAR_ID) to Goccia.Constants.PropertyNames and replace all hardcoded property-name literals in PlainMonthDay with the shared constants. Add month/monthCode consistency validation in both CoercePlainMonthDay and MonthDayWith — when both properties are present and conflict, throw RangeError instead of silently ignoring one. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@units/Goccia.Values.TemporalPlainMonthDay.pas`:
- Around line 105-108: The parser currently accepts any "M"+integer (e.g., "M1"
or "M001"); require the canonical monthCode format produced by GetMonthCode
instead: ensure MonthCodeStr[1] = 'M' and MonthCodeStr has exactly three
characters, verify the two trailing characters are digits (use TryStrToInt on
Copy(MonthCodeStr,2,2) into MonthPart) and validate MonthPart is in the 1..12
range; if any check fails call ThrowTypeError('Invalid monthCode for ' +
AMethod). Apply the same exact validation at the other parse site that uses
MonthCodeStr/MonthPart.
🪄 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: CHILL
Plan: Pro
Run ID: 57ebb989-359f-49e8-8e3e-528fc38425e5
📒 Files selected for processing (4)
tests/built-ins/Temporal/PlainMonthDay/prototype/equals.jstests/built-ins/Temporal/PlainMonthDay/prototype/with.jsunits/Goccia.Constants.PropertyNames.pasunits/Goccia.Values.TemporalPlainMonthDay.pas
✅ Files skipped from review due to trivial changes (2)
- tests/built-ins/Temporal/PlainMonthDay/prototype/with.js
- units/Goccia.Constants.PropertyNames.pas
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/built-ins/Temporal/PlainMonthDay/prototype/equals.js
Require exactly 3 characters (M + two digits) for monthCode instead of accepting any M+integer (e.g., M1, M001). Validate the parsed month is in 1..12 range. Applied to all three parse sites: CoercePlainMonthDay, MonthDayWith, and PlainMonthDayFrom. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
units/Goccia.Builtins.Temporal.pas (2)
923-944:⚠️ Potential issue | 🟠 Major
compare()is still narrower than the companionfrom()APIs.These coercers only accept existing values or strings, even though the new
from()routines already accept property bags.Temporal.PlainYearMonth.compare({ year: 2024, month: 1 }, '2024-02')and the analogousPlainMonthDaycall will throw here. Reuse the same coercion path asfrom()so the new statics stay internally consistent.Also applies to: 1053-1073
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.Temporal.pas` around lines 923 - 944, CoerceYearMonth only accepts TGocciaTemporalPlainYearMonthValue or ISO strings; update it to also accept property bags the same way the PlainYearMonth.from() path does: detect TGocciaObject/record-like inputs and route them through the same construction/parsing logic used by the static from routine (reuse the from() coercion path or shared helper used by from), calling the same internal parser/constructor instead of failing; apply the same change to the analogous CoerceMonthDay routine so both coercers accept value objects, strings, and property-bag inputs (reference: CoerceYearMonth, TryParseISOYearMonth, TGocciaTemporalPlainYearMonthValue.Create and the PlainYearMonth.from() code path).
877-885:⚠️ Potential issue | 🟠 MajorThe new
from()helpers dropoptions.overflowentirely.The overflow helper is already in scope, but neither function ever reads
AArgs[1], so{ overflow: 'constrain' }never takes effect. Inputs like{ year: 2024, month: 13 }forPlainYearMonth.from()or{ month: 2, day: 30 }forPlainMonthDay.from()still go straight to the constructors and throw instead of being clamped/rejected intentionally.Also applies to: 988-997
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.Temporal.pas` around lines 877 - 885, PlainYearMonthFrom and PlainMonthDayFrom currently ignore the second argument so options.overflow is never used; fix both functions to read AArgs.GetElement(1), extract the overflow option using the existing overflow helper in scope, and pass that overflow value into the code that constructs or normalizes the result (e.g., feed it into the PlainYearMonth/PlainMonthDay constructor or the normalization routine you call) so inputs like {year:2024,month:13} or {month:2,day:30} honor the overflow setting; specifically update TGocciaTemporalBuiltin.PlainYearMonthFrom and TGocciaTemporalBuiltin.PlainMonthDayFrom to consume AArgs[1] and apply the overflow behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@units/Goccia.Builtins.Temporal.pas`:
- Around line 898-909: The PlainYearMonth.from property-bag path currently
requires a numeric 'month' and rejects objects that only provide 'monthCode';
change the TGocciaObjectValue handling so it first checks for a 'monthCode'
property and, if present, parses it using the same canonical monthCode parser
used by PlainMonthDay.from (reuse that parser/function rather than duplicating
logic), apply the parsed numeric month when 'month' is absent, and if both
'monthCode' and 'month' are present verify they match and ThrowTypeError on
mismatch; update the branch that constructs TGocciaTemporalPlainYearMonthValue
to accept the resolved month and year (using Trunc(V.ToNumberLiteral.Value for
year) and the month resolved from either month or parsed monthCode).
- Around line 498-518: Replace hardcoded Temporal property strings and messages
in the PlainDate.from path by using the shared Temporal property-name constants:
use PROP_YEAR instead of 'year', PROP_MONTH instead of 'month', PROP_DAY instead
of 'day' (and PROP_MONTH_CODE where month-code access is needed) in all
Obj.GetProperty(...) calls and in the ThrowTypeError/ThrowRangeError messages;
update the calls and string concatenations around ThrowRangeError('Month ' +
IntToStr(Mo) + ' out of range') to reference PROP_MONTH (or include PROP_MONTH
in the message) so that GetProperty, ThrowTypeError and ThrowRangeError usages
in this block consistently use PROP_YEAR, PROP_MONTH, PROP_DAY (and
PROP_MONTH_CODE when applicable).
---
Duplicate comments:
In `@units/Goccia.Builtins.Temporal.pas`:
- Around line 923-944: CoerceYearMonth only accepts
TGocciaTemporalPlainYearMonthValue or ISO strings; update it to also accept
property bags the same way the PlainYearMonth.from() path does: detect
TGocciaObject/record-like inputs and route them through the same
construction/parsing logic used by the static from routine (reuse the from()
coercion path or shared helper used by from), calling the same internal
parser/constructor instead of failing; apply the same change to the analogous
CoerceMonthDay routine so both coercers accept value objects, strings, and
property-bag inputs (reference: CoerceYearMonth, TryParseISOYearMonth,
TGocciaTemporalPlainYearMonthValue.Create and the PlainYearMonth.from() code
path).
- Around line 877-885: PlainYearMonthFrom and PlainMonthDayFrom currently ignore
the second argument so options.overflow is never used; fix both functions to
read AArgs.GetElement(1), extract the overflow option using the existing
overflow helper in scope, and pass that overflow value into the code that
constructs or normalizes the result (e.g., feed it into the
PlainYearMonth/PlainMonthDay constructor or the normalization routine you call)
so inputs like {year:2024,month:13} or {month:2,day:30} honor the overflow
setting; specifically update TGocciaTemporalBuiltin.PlainYearMonthFrom and
TGocciaTemporalBuiltin.PlainMonthDayFrom to consume AArgs[1] and apply the
overflow 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: CHILL
Plan: Pro
Run ID: 62eb9eb8-8e74-412e-9f9c-fbb2b8b1e6ee
📒 Files selected for processing (3)
tests/built-ins/Temporal/PlainMonthDay/from.jsunits/Goccia.Builtins.Temporal.pasunits/Goccia.Values.TemporalPlainMonthDay.pas
✅ Files skipped from review due to trivial changes (1)
- tests/built-ins/Temporal/PlainMonthDay/from.js
🚧 Files skipped from review as they are similar to previous changes (1)
- units/Goccia.Values.TemporalPlainMonthDay.pas
…pport to PlainYearMonth.from
Replace all hardcoded property-name strings ('year', 'month', 'day', 'hour',
etc.) in Goccia.Builtins.Temporal with shared PROP_* constants from
Goccia.Constants.PropertyNames. Add time-related property constants
(PROP_HOUR through PROP_NANOSECOND). Add monthCode support to
PlainYearMonth.from() property-bag path with canonical format validation
and month/monthCode consistency checking, matching PlainMonthDay.from().
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Apr 16, 2026
This was referenced Apr 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.