fix(web): stop UNTIL from double-floating in the recurrence form - #2525
Merged
Conversation
Opening a timed recurring event whose rule has UNTIL crashed the whole app (React #185, max update depth exceeded) for any user off UTC. useRecurrence seeded its editable `until` state directly from CompassEventRRule's parsed `options.until`, which is already in the floating frame used for candidate expansion. Feeding that value back into a second CompassEventRRule floated it again, so every render emitted a rule shifted by one more timezone offset and the effect's deep-equal guard never converged. Un-float `until` (for timed events only - all-day UNTILs are never floated) before storing it as state, so the round trip through a new CompassEventRRule floats it exactly once. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
/simplify review (reuse + altitude passes) surfaced two issues in the prior commit's fix, both stemming from the same root cause: only the inbound seeding of `until` state was corrected, not the outbound value useRecurrence returns for display. - CompassEventRRule now exposes a public `until` getter that un-floats the internal value for timed events, mirroring the un-floating `all()` already does for its returned dates. This makes round-tripping `until` through a new instance idempotent by construction, instead of relying on every caller to know about and manually undo the float. - useRecurrence.ts uses that getter for both seeding and the returned `until` (previously `rrule.options.until`, still-floating). The previously-returned value fed EndsOnDate's DatePicker directly, so the "Ends on" date was showing a day earlier than selected for any non-UTC user on a timed recurring event - reproduced live (Denver host): picking July 31 displayed as "7-30-2026". - Drops the duplicated isTimed re-derivation in useRecurrence.ts (getCompassEventDateFormat round-trip) in favor of the class's own getter, which already has this correctly computed. - Lazily initializes the `until` state (useState(() => ...)) so the now-cheaper un-float only runs on mount, not every render. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Independent review flagged that the new until getter was only proven indirectly through the web-layer useRecurrence regression test. Adds direct core-package tests: un-floats a timed UNTIL to the real instant, returns null with no UNTIL, leaves an all-day UNTIL unmodified, and the getter's output is safe to round-trip back into a new instance without drifting. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
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.
Summary
Opening the event form on a timed recurring event whose rule has
UNTILcrashed the whole app for any user off UTC (minified React error #185, "Maximum update depth exceeded"). Reported via a series titled "Review KPIs, Weekly Review" that repeats until a fixed date - clicking it in the week grid took down the page.useRecurrenceseeded its editableuntilstate directly fromCompassEventRRule's parsedoptions.until, which is already in the "floating" frameCompassEventRRuleuses internally for candidate expansion (UTC calendar fields holding the wall-clock digits).CompassEventRRule(to rebuild the rule string as the user edits) caused#initOptionsto float it again.UNTILtherefore drifted by one timezone offset on every render. The sync effect's deep-equal guard never converged, so it kept callingsetDraft, which re-rendered the hook, which driftedUNTILagain - until React aborted at its nested-update limit and the error boundary took down the page.TZ=America/Denver, the reported rule'sUNTILwalked back 6 hours per render with no convergence.Fix, in two commits:
until(timed events only -#initOptionsnever floats all-dayUNTIL) before storing it as editable state, so the round trip through the rebuiltCompassEventRRulefloats it exactly once and stays stable./simplifyreview surfaced that only the inbound seeding was fixed - the hook's returneduntil(what feeds the "Ends on"DatePicker) still read the raw, still-floatingrrule.options.until, so the picker showed a day earlier than selected for non-UTC users. Pushed the un-floating into a new publicCompassEventRRule#untilgetter (mirroring the un-floatingall()already does for its returned dates) so round-trippinguntilis idempotent by construction for any caller, not just this one - anduseRecurrence.tsnow uses it for both directions, dropping a duplicatedisTimedre-derivation in the process.Simplicity
Ran
/simplify(reuse, simplification, efficiency, altitude passes) against the diff:useRecurrence.tswas re-deriving "is this event timed" via a date-string-format round-trip whendraft.values.schedule.kindalready had the answer. Superseded by pushing the fix intoCompassEventRRule#until, which removed the re-derivation entirely rather than just simplifying it.untilstate now uses React's lazyuseState(() => ...)initializer so the (now cheaper) un-float only runs on mount, not every render.useRecurrence.ts) was a bandaid on an asymmetric class API (CompassEventRRulefloatsuntilon the way in via#initOptionsbut didn't un-float it on the way out). Pushed the fix down into the class via a newuntilgetter, matching the precedent already set byall(). Confirmed via grep thatuseRecurrence.tsis the only production caller reading.options.until/.untiloff a constructed instance today, so this also forecloses the same bug for any future caller.Automated validation
Browser (anonymous/IndexedDB mode,
bun dev:web, real host timezoneAmerica/Denver- no override needed):Ends ondate.UNTILrule still applied, saved, reopened - no crash (exercises theisTimedguard's other branch).Independent review
Two independent passes, both via fresh subagents with no access to the implementation's own conclusions:
/simplify's 4 parallel angle-reviewers (reuse/simplification/efficiency/altitude) - findings applied, see Simplicity above.AGENTS.md, and the stated task intent (not told which findings had already been applied). Verified the root-cause claim against the actual code, confirmed the regression test uses a real (non-mocked)setDraftfeedback loop rather than the mocked pattern used elsewhere in the file, confirmed private field ordering (#isTimed/#timezone) is safe for the new getter, and checked every other productionCompassEventRRuleconstruction site is unaffected. No blocking findings. One coverage gap flagged (the new getter was only proven indirectly via the web-layer test) - addressed in a follow-up commit adding direct core-package unit tests.Test plan
cd packages/web && TZ=Etc/UTC NODE_ENV=test bun test src/views/Forms/EventForm/DateControlsSection/RecurrenceSection/- 23 passcd packages/core && TZ=Etc/UTC NODE_ENV=test bun test src/util/event/compass.event.rrule.test.ts- 27 passbun run type-check- cleanbiome checkon all touched files - clean