Fix automation timezone scheduling#3738
Conversation
📝 WalkthroughWalkthroughIntroduces timezone-aware formatting and RRULE behavior changes: adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes automation recurrence scheduling by switching rrule.js to floating wall-clock DTSTART (no TZID), converting each occurrence to real UTC via rruleDateToUtc, and displaying run times in the automation's configured timezone across desktop and CLI surfaces. It also fixes the --device omission bug in automations update. Confidence Score: 5/5Safe to merge; the recurrence timezone math is correct and well-tested, and all remaining findings are P2 defensive-coding suggestions. The floating-DTSTART approach is sound: wall-clock dates are stable across process timezones, each occurrence is correctly converted to real UTC via TZDate, and DST transitions are handled properly as confirmed by the new tests. The --device fix and IANA validation are correct. Both open comments are P2: the timezone-crash risk requires bad legacy DB data to trigger, and the HOURLY/DST note is a documentation suggestion. No P0/P1 defects found. packages/shared/src/rrule.ts — worth adding a comment near buildRuleString about HOURLY DST fall-back ambiguity for future maintainers.
|
| Filename | Overview |
|---|---|
| packages/shared/src/rrule.ts | Core fix: switches rrule.js to floating DTSTART (no TZID), converts each raw occurrence to real UTC via rruleDateToUtc, and adds formatDateTimeInTimezone. Logic is correct for DAILY/WEEKLY/MINUTELY; HOURLY across DST transitions has an inherent ambiguity worth documenting. |
| packages/trpc/src/router/automation/schema.ts | Adds runtime IANA timezone validation via Intl.DateTimeFormat; clean, correct approach with no issues. |
| packages/cli/src/commands/automations/update/command.ts | Fixes device-clearing bug: sends options.device (undefined when omitted) instead of options.device ?? null, so the server preserves existing targetHostId when --device is not passed. |
| packages/cli/src/commands/automations/format.ts | New shared formatter that safely handles null/undefined date and timezone values, falling back to UTC and em-dash appropriately. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/automations/$automationId/components/AutomationDetailSidebar/AutomationDetailSidebar.tsx | Replaces date-fns format (UTC-local) with formatDateTimeInTimezone (automation timezone); correct behaviour change, but no error boundary for invalid legacy timezone values in the DB. |
| packages/shared/src/rrule.test.ts | Adds well-targeted tests for the new UTC math and DST wall-clock stability; covers both PST→PDT transition and daily scheduling. |
| packages/cli/src/commands/automations/list/command.ts | Uses formatAutomationDate for consistent timezone-aware display in the list table; straightforward and correct. |
| packages/cli/src/commands/automations/create/command.ts | Switches next-run display from raw ISO string to timezone-aware formatAutomationDate; no issues. |
| packages/cli/src/commands/automations/resume/command.ts | Replaces toISOString() with formatAutomationDate for the resume confirmation message; correct and consistent with other CLI commands. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Input: rrule body + dtstart (real UTC) + timezone"] --> B["formatRRuleLocalDtstart\n(dtstart → wall-clock in automation TZ)"]
B --> C["buildRuleString\nDTSTART:{wall-clock} — no TZID\nRRULE:{rrule body}"]
C --> D["RRule.fromString(ruleString)"]
E["after (real UTC)"] --> F["utcToRruleDate(after, timezone)\n(real UTC → floating wall-clock)"]
F --> G["rule.after(floatingAfter, false)"]
D --> G
G --> H{"next occurrence\n(floating wall-clock)"}
H -->|null| I["throw: no future occurrences"]
H -->|Date| J["rruleDateToUtc(next, timezone)\n(wall-clock UTC fields → real UTC via TZDate)"]
J --> K["Return: plain Date in real UTC"]
K --> L["formatDateTimeInTimezone(date, timezone)\nIntl.DateTimeFormat in automation TZ"]
L --> M["Display: 'Apr 25, 2026 at 6:00 AM PDT'"]
Comments Outside Diff (1)
-
packages/shared/src/rrule.ts, line 347-353 (link)HOURLYrules have an inherent DST ambiguity worth documentingFor
FREQ=HOURLYwith a floating DTSTART, rrule.js generates occurrences one rrule-hour apart in floating space. During a DST fall-back hour (e.g.01:30LA occurs twice), both instances share the same floating representation, so one real UTC instant is silently lost. During spring-forward,TZDateadvances past the gap correctly but the cadence appears to skip an hour. This is an inherent trade-off of the floating-date approach; a short comment near this function would help future maintainers understand the expected behaviour.Prompt To Fix With AI
This is a comment left during a code review. Path: packages/shared/src/rrule.ts Line: 347-353 Comment: **`HOURLY` rules have an inherent DST ambiguity worth documenting** For `FREQ=HOURLY` with a floating DTSTART, rrule.js generates occurrences one rrule-hour apart in floating space. During a DST fall-back hour (e.g. `01:30` LA occurs twice), both instances share the same floating representation, so one real UTC instant is silently lost. During spring-forward, `TZDate` advances past the gap correctly but the cadence appears to skip an hour. This is an inherent trade-off of the floating-date approach; a short comment near this function would help future maintainers understand the expected behaviour. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/automations/$automationId/components/AutomationDetailSidebar/AutomationDetailSidebar.tsx
Line: 84-88
Comment:
**Unguarded timezone string passed to `Intl.DateTimeFormat`**
`formatDateTimeInTimezone` calls `new Intl.DateTimeFormat(locale, { timeZone: timezone })` directly. If `automation.timezone` on a legacy row contains a value the runtime rejects (e.g. a renamed IANA zone or corrupted data), `Intl.DateTimeFormat` throws a `RangeError` that propagates unhandled through the JSX tree and crashes the sidebar. The new schema `.refine(isValidIanaTimezone)` prevents future bad writes, but existing rows are not retroactively validated. Adding a try-catch inside `formatDateTimeInTimezone` or a React error boundary around the sidebar would prevent a crash for edge-case legacy data.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/shared/src/rrule.ts
Line: 347-353
Comment:
**`HOURLY` rules have an inherent DST ambiguity worth documenting**
For `FREQ=HOURLY` with a floating DTSTART, rrule.js generates occurrences one rrule-hour apart in floating space. During a DST fall-back hour (e.g. `01:30` LA occurs twice), both instances share the same floating representation, so one real UTC instant is silently lost. During spring-forward, `TZDate` advances past the gap correctly but the cadence appears to skip an hour. This is an inherent trade-off of the floating-date approach; a short comment near this function would help future maintainers understand the expected behaviour.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Fix automation timezone scheduling" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ? formatDateTimeInTimezone( | ||
| new Date(automation.nextRunAt), | ||
| automation.timezone, | ||
| ) | ||
| : "—" |
There was a problem hiding this comment.
Intl.DateTimeFormat
formatDateTimeInTimezone calls new Intl.DateTimeFormat(locale, { timeZone: timezone }) directly. If automation.timezone on a legacy row contains a value the runtime rejects (e.g. a renamed IANA zone or corrupted data), Intl.DateTimeFormat throws a RangeError that propagates unhandled through the JSX tree and crashes the sidebar. The new schema .refine(isValidIanaTimezone) prevents future bad writes, but existing rows are not retroactively validated. Adding a try-catch inside formatDateTimeInTimezone or a React error boundary around the sidebar would prevent a crash for edge-case legacy data.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/automations/$automationId/components/AutomationDetailSidebar/AutomationDetailSidebar.tsx
Line: 84-88
Comment:
**Unguarded timezone string passed to `Intl.DateTimeFormat`**
`formatDateTimeInTimezone` calls `new Intl.DateTimeFormat(locale, { timeZone: timezone })` directly. If `automation.timezone` on a legacy row contains a value the runtime rejects (e.g. a renamed IANA zone or corrupted data), `Intl.DateTimeFormat` throws a `RangeError` that propagates unhandled through the JSX tree and crashes the sidebar. The new schema `.refine(isValidIanaTimezone)` prevents future bad writes, but existing rows are not retroactively validated. Adding a try-catch inside `formatDateTimeInTimezone` or a React error boundary around the sidebar would prevent a crash for edge-case legacy data.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Addressed in 00ea424: formatDateTimeInTimezone now catches invalid timezone values and falls back to UTC formatting, so legacy/corrupt rows do not crash the sidebar.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/shared/src/rrule.test.ts (1)
296-296: Nit: preferinstanceof Dateover constructor-name comparison.
expect(next.constructor.name).toBe("Date")is a brittle stand-in for aninstanceofcheck — minified code or Date subclasses (notablyTZDate, which is what this PR is specifically removing here) would also satisfy this string equality. The point of the assertion is to verify a plainDate, so use the more idiomatic check.♻️ Proposed change
- expect(next.constructor.name).toBe("Date"); + expect(next).toBeInstanceOf(Date); + // Tighter check that this is not a TZDate subclass: + expect(Object.getPrototypeOf(next)).toBe(Date.prototype);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/rrule.test.ts` at line 296, Replace the brittle constructor-name comparison in the test (expect(next.constructor.name).toBe("Date")) with an instanceof/Jest matcher check to assert a real Date object; update the assertion in packages/shared/src/rrule.test.ts to use expect(next).toBeInstanceOf(Date) (or expect(next instanceof Date).toBe(true)) so the test verifies a Date instance robustly.packages/trpc/src/router/automation/schema.ts (1)
17-30: Optional: improve timezone validation clarity.The
new Intl.DateTimeFormat(...)call relies on a side effect (throwing on invalid input). Consider one of these improvements for clarity:
Explicit side-effect handling: Assign to a throwaway or wrap with
void:function isValidIanaTimezone(timezone: string): boolean { try { void new Intl.DateTimeFormat(undefined, { timeZone: timezone }); return true; } catch { return false; } }Stricter IANA-only validation (Node.js 20+): Use
Intl.supportedValuesOf("timeZone")for direct lookups without exception handling:const IANA_ZONES = new Set(Intl.supportedValuesOf("timeZone")); function isValidIanaTimezone(timezone: string): boolean { return IANA_ZONES.has(timezone); }Both approaches avoid exception-based control flow. Current behavior is correct, but these patterns are clearer and slightly more performant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/router/automation/schema.ts` around lines 17 - 30, The isValidIanaTimezone function relies on an exception side-effect — update it to make the intent explicit by either (A) using a throwaway evaluation (e.g., prefix the constructor call with void) inside the try block so it’s clear you’re only testing validity, or (B) if targeting Node 20+/browsers that support it, build a Set from Intl.supportedValuesOf("timeZone") and replace isValidIanaTimezone to check membership in that Set; apply the chosen change in the isValidIanaTimezone function and keep the zod schema constant (the iana constant) so the refine still uses the new validator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/shared/src/rrule.ts`:
- Around line 423-438: Tests expecting the string built by
formatDateTimeInTimezone must be updated to match the actual Intl.DateTimeFormat
output (which includes comma separators for en-US) — replace expectations that
look for the " at " separator with the comma-separated form such as "Apr 25,
2026, 6:00 AM PDT" (and similar locale-specific variants used in the
assertions), leaving the formatDateTimeInTimezone function's explicit options
unchanged so the formatter remains stable.
---
Nitpick comments:
In `@packages/shared/src/rrule.test.ts`:
- Line 296: Replace the brittle constructor-name comparison in the test
(expect(next.constructor.name).toBe("Date")) with an instanceof/Jest matcher
check to assert a real Date object; update the assertion in
packages/shared/src/rrule.test.ts to use expect(next).toBeInstanceOf(Date) (or
expect(next instanceof Date).toBe(true)) so the test verifies a Date instance
robustly.
In `@packages/trpc/src/router/automation/schema.ts`:
- Around line 17-30: The isValidIanaTimezone function relies on an exception
side-effect — update it to make the intent explicit by either (A) using a
throwaway evaluation (e.g., prefix the constructor call with void) inside the
try block so it’s clear you’re only testing validity, or (B) if targeting Node
20+/browsers that support it, build a Set from
Intl.supportedValuesOf("timeZone") and replace isValidIanaTimezone to check
membership in that Set; apply the chosen change in the isValidIanaTimezone
function and keep the zod schema constant (the iana constant) so the refine
still uses the new validator.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6cde737a-98a4-4ba3-940a-38ed7fdfffcd
📒 Files selected for processing (9)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/automations/$automationId/components/AutomationDetailSidebar/AutomationDetailSidebar.tsxpackages/cli/src/commands/automations/create/command.tspackages/cli/src/commands/automations/format.tspackages/cli/src/commands/automations/list/command.tspackages/cli/src/commands/automations/resume/command.tspackages/cli/src/commands/automations/update/command.tspackages/shared/src/rrule.test.tspackages/shared/src/rrule.tspackages/trpc/src/router/automation/schema.ts
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/shared/src/rrule.test.ts (2)
313-313: PrefertoBeInstanceOf(Date)overconstructor.name.
expect(next.constructor.name).toBe("Date")is a string-based check that won't survive minification and is non-idiomatic for the test intent ("plain UTC Date instances").toBeInstanceOfmatches the matcher API used elsewhere and gives a better failure message.- expect(next.constructor.name).toBe("Date"); + expect(next).toBeInstanceOf(Date);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/rrule.test.ts` at line 313, Replace the string-based constructor name assertion with the instance matcher: in the test where you currently use expect(next.constructor.name).toBe("Date"), change it to use expect(next).toBeInstanceOf(Date) so the check is robust against minification and matches the project's matcher style.
14-29: Helper assertions are too loose to catch wrong hour/day digits.
expectDateTimePartschecks each expected piece independently withtoContain, which can yield false positives when one part's value is a substring of another. For example,hour: "6"is also a substring of year"2026", andday: "8"would be a substring of"2026"too — so a bug that produced the wrong hour/day could still pass these tests as long as the digit appears anywhere else in the formatted string (e.g., the year). It happens to work today only because the chosen years/days don't collide with the asserted hour/minute.Consider asserting the structured time together (e.g.,
"6:00 AM") or usingIntl.DateTimeFormat#formatToPartsso each component is matched against its own slot rather than a substring of the whole string.♻️ Sketch of a stricter helper
-function expectDateTimeParts( - formatted: string, - expected: { - month: string; - day: string; - year: string; - hour: string; - minute: string; - dayPeriod?: string; - timeZoneName: string; - }, -): void { - for (const value of Object.values(expected)) { - expect(formatted).toContain(value); - } -} +function expectDateTimeParts( + formatted: string, + expected: { + month: string; + day: string; + year: string; + hour: string; + minute: string; + dayPeriod?: string; + timeZoneName: string; + }, +): void { + // Assert the time slot as a unit so a wrong hour can't be masked by year digits. + const time = `${expected.hour}:${expected.minute}`; + expect(formatted).toContain(time); + expect(formatted).toContain(expected.month); + expect(formatted).toContain(expected.year); + expect(formatted).toContain(expected.timeZoneName); + if (expected.dayPeriod) expect(formatted).toContain(expected.dayPeriod); + // Day: match with a separator to avoid accidental substring hits. + expect(formatted).toMatch(new RegExp(`\\b${expected.day}\\b`)); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/rrule.test.ts` around lines 14 - 29, The helper expectDateTimeParts is too loose because it checks each expected piece with string containment, allowing false positives when parts (e.g., hour "6") appear elsewhere (e.g., year "2026"); update expectDateTimeParts to assert components in their own slots by using Intl.DateTimeFormat.prototype.formatToParts (or by asserting a combined time substring like "6:00 AM") and compare each named part (month, day, year, hour, minute, dayPeriod, timeZoneName) to the expected values rather than using toContain on the full formatted string; locate and change the implementation in expectDateTimeParts to parse formatToParts output and match parts by their type (e.g., "hour","minute","day","month","year","dayPeriod","timeZoneName") or build and assert a structured time token instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/shared/src/rrule.test.ts`:
- Line 313: Replace the string-based constructor name assertion with the
instance matcher: in the test where you currently use
expect(next.constructor.name).toBe("Date"), change it to use
expect(next).toBeInstanceOf(Date) so the check is robust against minification
and matches the project's matcher style.
- Around line 14-29: The helper expectDateTimeParts is too loose because it
checks each expected piece with string containment, allowing false positives
when parts (e.g., hour "6") appear elsewhere (e.g., year "2026"); update
expectDateTimeParts to assert components in their own slots by using
Intl.DateTimeFormat.prototype.formatToParts (or by asserting a combined time
substring like "6:00 AM") and compare each named part (month, day, year, hour,
minute, dayPeriod, timeZoneName) to the expected values rather than using
toContain on the full formatted string; locate and change the implementation in
expectDateTimeParts to parse formatToParts output and match parts by their type
(e.g., "hour","minute","day","month","year","dayPeriod","timeZoneName") or build
and assert a structured time token instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb03ec17-9f85-4864-acf3-5ba8858f25d9
📒 Files selected for processing (2)
packages/shared/src/rrule.test.tspackages/shared/src/rrule.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/shared/src/rrule.ts
* Fix automation timezone scheduling * Stabilize automation timezone formatting tests
PR1〜PR5 (#435 #436 #437 #438 #440) で 13 commits 全件 cherry-pick + 手動 conflict 解消で取り込み済み。 本コミットは git 履歴上 behind=0 とするための ours マージ記録。 取り込み済み 13 commits: - 1f55c62 Fix host service restart adoption (superset-sh#3732) - 0fe65d2 test(desktop): remove host-service-coordinator test (superset-sh#3734) - 3012b5a Add optimistic Electric collection updates (superset-sh#3722) - c272a51 fix(desktop): drop branch row from v2 sidebar workspace item (superset-sh#3733) - c2f3fdc feat(desktop): add fade-edge mask utilities (superset-sh#3735) - 682d07c fix v2 terminal osc links (superset-sh#3736) - 7c0d22b feat(ports): surface remote host-service ports in the sidebar (superset-sh#3676) - 6a3be2d [codex] Stabilize v2 terminal resize (superset-sh#3739) - 8928ac6 [codex] Improve v2 pane header responsiveness (superset-sh#3737) - 5fe3d22 refactor(desktop): tidy v2 terminal session dropdown (superset-sh#3743) - 66c23d6 Fix automation timezone scheduling (superset-sh#3738) - 16e270c [codex] Add terminal session titles (superset-sh#3740) - 583fa5d fix(desktop): refit v2 terminal after font settle (superset-sh#3742)
1 participant
Summary
automations updateomits--deviceVerification
bun test packages/shared/src/rrule.test.tsTZ=UTC bun test packages/shared/src/rrule.test.ts -t "recurrence timezone math"TZ=America/Los_Angeles bun test packages/shared/src/rrule.test.ts -t "recurrence timezone math"bun run --cwd packages/shared typecheckbun run --cwd packages/trpc typecheckbun run --cwd packages/cli typecheckbun run --cwd apps/desktop typecheckbunx @biomejs/biome@2.4.2 check packages/shared/src/rrule.ts packages/shared/src/rrule.test.ts packages/trpc/src/router/automation/schema.ts packages/cli/src/commands/automations/format.ts packages/cli/src/commands/automations/create/command.ts packages/cli/src/commands/automations/list/command.ts packages/cli/src/commands/automations/resume/command.ts packages/cli/src/commands/automations/update/command.ts 'apps/desktop/src/renderer/routes/_authenticated/_dashboard/automations/$automationId/components/AutomationDetailSidebar/AutomationDetailSidebar.tsx'\n\n## Follow-up\nExisting production automations whosenext_run_atwas already computed with the old logic should be recomputed fromrrule,dtstart, andtimezoneafter this deploy. This PR prevents new creates/updates/resumes and future cron advances from carrying the offset bug forward.Summary by cubic
Fixes automation scheduling so runs occur at the intended local time, regardless of host timezone or UTC drift. Times now display in the automation’s timezone in both desktop and CLI.
Bug Fixes
formatDateTimeInTimezone; added CLIformatAutomationDate.automations updateno longer clears the target device when--deviceis omitted.Migration
next_run_atfromrrule,dtstart, andtimezoneafter deploy.Written for commit 00ea424. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests