Repository navigation
[scheduler] Read the timestamps of the processed dates in the recurring events - #24004
Conversation
…ng events The recurring events read the instants and the timezones of the processed dates they receive, instead of calling `getTime()` and `getTimezone()` on their adapter values. `RecurringSeries` takes the timestamp of its first occurrence and its timezone, the expansion and the range index take the timestamps of the visible range, and the edits and deletions use the data timezone of the event. The dependency engine adds its day lags with the `addDaysOnWallClock()` of `plain-date-utils`, and keeps returning `NaN` past the range of `Date`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
flaviendelangle
added
type: enhancement
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
…urrences `decideSplitRRule()` takes the processed event and builds its series with `RecurringSeries.fromEvent()`. `getOccurrencesFromEvents()` takes the timestamps of its range, so the days of a view no longer create two adapter values to read their instants back. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
rita-codes
left a comment
rita-codes
left a comment
There was a problem hiding this comment.
PR review
Nothing is merge-blocking. The "no behavior change" claim holds: every comparison and bound I traced is operator-for-operator equivalent to master, the store relabels occurrenceStart and changes into the data timezone before the plugin runs, and the full scheduler jsdom suites pass (3,939 tests). The new addDependencyLag test does fail without the range check, as claimed. What remains is a hand-rolled copy of two existing helpers, a constant whose new name says the opposite of what it compares, and one invariant the PR now relies on without a test.
Bugs (1)
1. ℹ️ The delete helpers now mix the label timezone of occurrenceStart with the data timezone
Location: packages/x-scheduler-internals-premium/src/internals/utils/recurring-events/deleteRecurringEvent.ts:57
createStartOfPlainDate(
adapter,
buildTemporalPlainDate(adapter, occurrenceStart),
originalEvent.dataTimezone.timezone,
),buildTemporalPlainDate reads the calendar date in the timezone occurrenceStart is labeled with, and createStartOfPlainDate builds midnight of that date in the data timezone. Master used the label for both. Today the two agree because the store relabels occurrenceStart with adapter.setTimezone(occurrenceStart, original.dataTimezone.timezone) before calling the plugin, so this is not reachable, but the correctness of the EXDATE and UNTIL instants now depends on that relabel rather than on the helper itself.
Failure scenario: A future direct caller passes a display-timezone value (data New York, display Tokyo, occurrence 22:00 NY = 11:00 Tokyo the next day): EXDATE lands on the wrong day's midnight.
Fix: Read the plain date in the data timezone too: toPlainDateTime(adapter.getTime(occurrenceStart), dataTimezone).date, or keep as is and leave the existing "Expects occurrenceStart labeled in the event's data timezone" comment, which is now load-bearing.
Tests (2)
1. 🟡 Nothing pins that excluded occurrences still count toward COUNT at the split
Location: packages/x-scheduler-internals-premium/src/internals/utils/recurring-events/updateRecurringEvent.ts:366
// The excluded occurrences still count toward COUNT.
const series = RecurringSeries.fromEvent(adapter, originalEvent);Master built the series without EXDATEs. fromEvent passes event.dataTimezone.exDates, and the behavior is unchanged only because countBefore() never consults excludedDays. The only decideSplitRRule COUNT test (count 42 → 37) uses an event without EXDATE, and the only EXDATE test in the file uses a rule without COUNT, so remaining is never computed with both.
Failure scenario: A later "fix" making countBefore() skip excluded days (plausible, since hasOccurrence* already ignores them) silently inflates the new series' COUNT after a this-and-following edit, and no split test notices.
Fix: In describe('decideSplitRRule'), DAILY count 5 from Jan 1 with EXDATE Jan 2, split at Jan 4 → expect { count: 2 }.
2. ℹ️ The decideSplitRRule fixture labels the event 'system', which production never produces
Location: packages/x-scheduler-internals-premium/src/internals/utils/recurring-events/updateRecurringEvent.test.ts:26
timezone: adapter.getTimezone(originalSeriesStart) yields 'system' for a 'default'-labeled start, while real events carry dataTimezone.timezone === 'default'. The engine treats both the same, so no gap, but the fixture reproduces the pre-PR adapter detour instead of the new contract. timezone: defaultEvent.dataTimezone.timezone is the honest fixture.
Simplifications (2)
1. 🟡 useEventOccurrencesGroupedByDay rebuilds getStartOfDayMs and getEndOfDayMs by hand
Location: packages/x-scheduler-internals/src/use-event-occurrences-grouped-by-day/useEventOccurrencesGroupedByDay.ts:91
start: toEpochMs(plainDateFromDayNumber(dayNumbers[0]), 0, displayTimezone),
// The last instant before the day after the last one starts.
end: toEpochMs(plainDateFromDayNumber(lastDayNumber + 1), 0, displayTimezone) - 1,Both lines are the bodies of helpers already exported from the barrel: getStartOfDayMs and getEndOfDayMs in plain-date-utils/timeZone.ts, including the - 1 and the DST comment.
Fix:
start: getStartOfDayMs(days[0].timestamp, displayTimezone),
end: getEndOfDayMs(days[days.length - 1].timestamp, displayTimezone),Removes lastDayNumber, the comment, two PlainDate allocations and the plainDateFromDayNumber/toEpochMs imports.
2. ℹ️ The range guard could live in addDaysOnWallClock() itself
Location: packages/x-scheduler-internals-premium/src/internals/utils/dependency-utils.ts:59
addDaysOnWallClock has a second caller in useDayCellDropTarget.ts that would get the same Intl throw on an absurd result. A one-line check inside the shared helper would delete addDaysInDateRange, the constant and its comments here. The PR body argues for keeping it in the engine, so this is a judgement call.
Docs (2)
1. 🟡 MAX_WALL_CLOCK_MS now bounds an instant, not a wall-clock time
Location: packages/x-scheduler-internals-premium/src/internals/utils/dependency-utils.ts:53
// The range of `Date`, minus a margin for the offsets of the timezone read around the result.
const MAX_WALL_CLOCK_MS = 8.64e15 - 2 * MS_PER_DAY;
...
return Math.abs(epochMs + dayCount * MS_PER_DAY) > MAX_WALL_CLOCK_MSMaster compared toLocalMs(epochMs, tz) + days under the name MAX_LOCAL_MS. The PR renames it to "wall clock" while switching the operand to an epoch instant, which the PR body itself states ("adds the lag to the instant rather than to the wall-clock time"). The JSDoc "where the offsets of the timezone can't be read" is also only true for IANA zones: for 'UTC' the helper returns a finite out-of-range number without throwing, and the guard is what makes the result a uniform NaN.
Fix: MAX_EPOCH_MS (or MAX_DATE_MS), and "…or returns NaN past the range of Date, where Intl throws instead of returning the offsets of an IANA timezone."
2. ℹ️ The plugin contract says "instant" without saying timestamp or inclusive
Location: packages/x-scheduler-internals/src/internals/plugins/SchedulerRecurringEventsPlugin.types.ts:31
"from the instant start to the instant end" on a bare number pair. Both bounds are inclusive (the range index uses <=/>=, the expansion scans the whole day of end, and the grouped-by-day caller passes nextDayStart - 1 for that reason). event-utils.ts already says "The first instant of the range, as a timestamp" / "The last instant…"; the cross-package contract deserves the same, plus "both included". Same wording in getRecurringEventOccurrencesForVisibleDays and getEventsForRange.
Verdict
Approve after nits — no regression, but the hand-rolled day bounds and the misleading constant name are worth a pass before merge.
🤖 Review generated with Claude Code
…OUNT of a split - `useEventOccurrencesGroupedByDay()` reads the bounds of its days with `getStartOfDayMs()` and `getEndOfDayMs()`. - The range check of the dependency engine names its constant `MAX_EPOCH_MS`, as it bounds an instant. - The contracts of the visible range say that both bounds are timestamps and included. - A new test checks that the excluded occurrences still count toward the COUNT of the series a split creates, and the fixture of the split tests uses the `'default'` timezone of real events. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
rita-codes
left a comment
rita-codes
left a comment
There was a problem hiding this comment.
Follow-up verified: getStartOfDayMs/getEndOfDayMs in the grouped-by-day hook, the MAX_EPOCH_MS rename and JSDoc, the "both included" range docs and the EXDATE+COUNT split test (COUNT 2 is right). Full scheduler suite green locally. Thanks!
This PR moves the internal inputs of the recurring events engine from adapter values to timestamps. The series, the expansion of the visible range, the deletions and the edits read the instants and the timezones the processed events already carry, instead of reading them back from adapter values with
getTime()andgetTimezone(). The dependency engine uses theaddDaysOnWallClock()ofplain-date-utilsinstead of its own copy. The public API doesn't change.Changes
RecurringSeriestakes the instant of DTSTART as a timestamp, and its timezone becomes required.RecurringSeries.fromEvent()passes the timestamp of the processed start, anddecideSplitRRule()takes the timestamp and the timezone of the series instead of an adapter value.getOccurrencesFromEvents()reads its two bounds once and passes them to the event range index and togetOccurrencesForVisibleDays(). The range index read them on every query, and the expansion on every recurring event. The range index no longer needs the adapter.addDaysOnWallClock()ofplain-date-utils. It keeps its range check, which returnsNaNpast the range ofDate, whereIntlthrows instead of returning the offsets of a timezone. The other difference between the two helpers, the shortcut for 0 days, can't be reached: a lag of 0 is no lag.Behavior changes
None. The engine reads the same instants and timezones as before: the timestamp of a processed date is the
getTime()of its value, and the data timezone is the one its values are labeled with.getTimezone()returns'system'for a value labeled'default', and both read the offsets of the system.The range check of the dependency engine now adds the lag to the instant rather than to the wall-clock time. The two differ by less than a day, and only matter at the edge of the range of
Date, in the year 275,760.Bundle size
Measured from source with esbuild, minified, with third-party packages external and without the error minifier, gzip level 9.
event-calendarevent-calendar-premiumevent-timeline-premiumAdapter usage
Counted like in #23943. The references go from 234 to 226.
getTimegetTimezonegetOccurrencesFromEvents()gains the twogetTime()calls that the range index and the expansion lose. The references left in the recurring events engine read values of the public API: the UNTIL of the processed rule, the EXDATE of the processed event, and the datesupdateRecurringEvent()receives.Benchmarks
Run in jsdom with
pnpm vitest bench, master then this PR, on a machine running other test suites. Operations per second, higher is better.defaultdata timezoneThe differences stay within the noise of the runs: the linear queries of the range index benchmark, whose code this PR doesn't change, vary by up to 44% between the two runs. The range index benchmark reads the timestamps of its bounds at each query, so it measures the same work as before.
Follow-ups
getOccurrencesFromEvents(), the occurrence selectors anduseEventOccurrencesGroupedByDay(), can pass the timestamps of their days. That removes its twogetTime()calls, anduseEventOccurrencesGroupedByDay()no longer has to create two adapter values just to pass them.Validation
The tests of the recurring and dependency engines, auto-scheduling, the range index, the occurrence hooks and the Timeline dependencies pass in jsdom (1,106 tests) and in the browser (349 tests). CI runs the full suites.
The tests of the series, the expansion, the split rules and the range index pass timestamps.
A new test of
addDependencyLag()checks that a lag in days or weeks past the range ofDatereturnsNaN, in UTC and in America/New_York. It fails without the range check. The existing tests didn't pin it: they run in the timezone of the system, whose offsets come out asNaNpast the range anyway.On systems in Pacific/Apia, Australia/Lord_Howe and America/Santiago, the tests of the recurring and dependency engines give the same results as on master, where 39 of them expect a UTC system.
A
/simplifypass with four reviews (reuse, simplification, efficiency, altitude):decideSplitRRule()takes the processed event and builds its series withRecurringSeries.fromEvent(), andgetOccurrencesFromEvents()takes the timestamps of its range, so the days of a view no longer create two adapter values to read their instants back. The range check of the dependency engine stays: it existed on master, and dropping it would letIntlthrow on an absurd lag.A review round with Codex and the base-ui-review skill approved without findings.
Rita's review:
useEventOccurrencesGroupedByDay()reads the bounds of its days withgetStartOfDayMs()andgetEndOfDayMs(), the constant of the range check isMAX_EPOCH_MS, as it bounds an instant, and the contracts of the range say that both bounds are timestamps and included. A new test checks that the excluded occurrences still count toward the COUNT of the series a split creates, and the fixture of the split tests uses the'default'timezone of real events. The delete helpers still read the day ofoccurrenceStartin its own timezone: their JSDoc says it must be in the data timezone, and the store passes it so. The range check stays in the dependency engine.TypeScript, ESLint and Prettier pass.
I have followed (at least) the PR section of the contributing guide.
🤖 Generated with Claude Code