From 0c751f46a3399092a018613100587bcf9cb8d431 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 30 May 2026 21:44:43 -0700 Subject: [PATCH] fix: address review findings (#3983) --- .../automations/AutomationCustomCronPanel.tsx | 148 ++++++++++++++++++ .../AutomationSchedulePicker.test.ts | 102 ++++++++++++ .../automations/AutomationSchedulePicker.tsx | 100 ++++++------ .../automations/AutomationsPage.tsx | 17 +- src/shared/automation-schedules.test.ts | 14 ++ src/shared/automation-schedules.ts | 21 +++ 6 files changed, 342 insertions(+), 60 deletions(-) create mode 100644 src/renderer/src/components/automations/AutomationCustomCronPanel.tsx create mode 100644 src/renderer/src/components/automations/AutomationSchedulePicker.test.ts diff --git a/src/renderer/src/components/automations/AutomationCustomCronPanel.tsx b/src/renderer/src/components/automations/AutomationCustomCronPanel.tsx new file mode 100644 index 000000000..64c7554ff --- /dev/null +++ b/src/renderer/src/components/automations/AutomationCustomCronPanel.tsx @@ -0,0 +1,148 @@ +import React from 'react' +import { CheckCircle2, CircleAlert, RotateCcw } from 'lucide-react' +import { Button } from '@/components/ui/button' +import { Input } from '@/components/ui/input' +import { cn } from '@/lib/utils' +import { formatAutomationSchedule } from '../../../../shared/automation-schedules' +import type { AutomationDraft } from './AutomationEditorDialog' +import { Field } from './automation-page-parts' + +const FIELD_CONTROL_CLASS = 'border-input bg-input/30 shadow-xs dark:bg-input/30' + +export const AUTOMATION_CRON_QUICK_STARTS = [ + { label: 'Every 15 min', expression: '*/15 * * * *' }, + { label: 'Hourly workday', expression: '0 9-17 * * 1-5' }, + { label: 'Weekdays 9 AM', expression: '0 9 * * 1-5' }, + { label: 'Monthly audit', expression: '0 9 1 * *' } +] as const + +export const AUTOMATION_CRON_FIELD_LABELS = ['Minute', 'Hour', 'Day', 'Month', 'Weekday'] as const + +export function getCronScheduleStatusLabel( + schedule: string, + validateSchedule: (schedule: string) => boolean +): { kind: 'empty' | 'invalid' | 'valid'; label: string } { + const trimmed = schedule.trim() + if (!trimmed) { + return { kind: 'empty', label: 'Choose a quick start or enter a five-field cron.' } + } + if (!validateSchedule(trimmed)) { + return { kind: 'invalid', label: 'Enter a valid five-field cron before saving.' } + } + const formatted = formatAutomationSchedule(trimmed) + return { kind: 'valid', label: formatted === 'Custom schedule' ? 'Valid custom cron' : formatted } +} + +export function getCronFieldValues(schedule: string): readonly string[] { + const parts = schedule.trim().split(/\s+/).filter(Boolean) + return AUTOMATION_CRON_FIELD_LABELS.map((_, index) => parts[index] ?? '...') +} + +export function AutomationCustomCronPanel({ + draft, + customScheduleInvalid, + validateAdvancedSchedule, + onUseSimpleSchedule, + onDraftChange +}: { + draft: AutomationDraft + customScheduleInvalid: boolean + validateAdvancedSchedule: (schedule: string) => boolean + onUseSimpleSchedule: () => void + onDraftChange: (updater: (current: AutomationDraft) => AutomationDraft) => void +}): React.JSX.Element { + const customScheduleStatus = getCronScheduleStatusLabel( + draft.customSchedule, + validateAdvancedSchedule + ) + const cronFieldValues = getCronFieldValues(draft.customSchedule) + + return ( +
+
+
+
Quick starts
+ +
+
+ {AUTOMATION_CRON_QUICK_STARTS.map((preset) => ( + + ))} +
+
+ + + onDraftChange((current) => ({ + ...current, + customSchedule: event.target.value, + scheduleWarning: null + })) + } + /> +
+ {AUTOMATION_CRON_FIELD_LABELS.map((label, index) => ( +
+
{label}
+
+ {cronFieldValues[index]} +
+
+ ))} +
+
+ {customScheduleStatus.kind === 'invalid' ? ( + + ) : ( + + )} + {customScheduleStatus.label} +
+ +
+ ) +} diff --git a/src/renderer/src/components/automations/AutomationSchedulePicker.test.ts b/src/renderer/src/components/automations/AutomationSchedulePicker.test.ts new file mode 100644 index 000000000..ffcd5a3cb --- /dev/null +++ b/src/renderer/src/components/automations/AutomationSchedulePicker.test.ts @@ -0,0 +1,102 @@ +import React from 'react' +import { renderToStaticMarkup } from 'react-dom/server' +import { describe, expect, it } from 'vitest' +import type { AutomationDraft } from './AutomationEditorDialog' +import { + AUTOMATION_CRON_QUICK_STARTS, + AutomationCustomCronPanel, + getCronFieldValues, + getCronScheduleStatusLabel +} from './AutomationCustomCronPanel' +import { + AUTOMATION_SCHEDULE_PRESET_OPTIONS, + getSchedulePresetDraft +} from './AutomationSchedulePicker' +import { isValidAutomationCronSchedule } from '../../../../shared/automation-schedules' + +const BASE_DRAFT: AutomationDraft = { + name: '', + prompt: '', + agentId: 'codex', + projectId: '', + workspaceMode: 'existing', + workspaceId: '', + baseBranch: '', + reuseSession: false, + preset: 'weekdays', + time: '09:15', + dayOfWeek: '1', + customSchedule: '', + missedRunGraceMinutes: '720', + scheduleWarning: null +} + +describe('AutomationSchedulePicker', () => { + it('offers custom cron as a selectable cadence', () => { + expect(AUTOMATION_SCHEDULE_PRESET_OPTIONS).toContainEqual(['custom', 'Custom cron']) + }) + + it('includes quick starts that are valid cron schedules', () => { + expect(AUTOMATION_CRON_QUICK_STARTS.length).toBeGreaterThan(0) + expect( + AUTOMATION_CRON_QUICK_STARTS.every((preset) => + isValidAutomationCronSchedule(preset.expression) + ) + ).toBe(true) + }) + + it('seeds custom cron from the current simple schedule', () => { + expect(getSchedulePresetDraft(BASE_DRAFT, 'custom')).toMatchObject({ + preset: 'custom', + customSchedule: '15 9 * * 1-5', + scheduleWarning: null + }) + }) + + it('preserves an existing custom cron when toggling back to custom', () => { + expect( + getSchedulePresetDraft({ ...BASE_DRAFT, customSchedule: '*/30 9-17 * * 1-5' }, 'custom') + ).toMatchObject({ + preset: 'custom', + customSchedule: '*/30 9-17 * * 1-5' + }) + }) + + it('summarizes custom cron validity for the inline status row', () => { + expect(getCronScheduleStatusLabel('', isValidAutomationCronSchedule)).toEqual({ + kind: 'empty', + label: 'Choose a quick start or enter a five-field cron.' + }) + expect(getCronScheduleStatusLabel('not cron', isValidAutomationCronSchedule)).toEqual({ + kind: 'invalid', + label: 'Enter a valid five-field cron before saving.' + }) + expect(getCronScheduleStatusLabel('0 9 * * 1-5', isValidAutomationCronSchedule)).toMatchObject({ + kind: 'valid' + }) + }) + + it('splits cron expressions into labeled field values', () => { + expect(getCronFieldValues('0 9 * * 1-5')).toEqual(['0', '9', '*', '*', '1-5']) + expect(getCronFieldValues('0 9')).toEqual(['0', '9', '...', '...', '...']) + }) + + it('renders quick starts beside the cron expression field', () => { + const markup = renderToStaticMarkup( + React.createElement(AutomationCustomCronPanel, { + draft: { ...BASE_DRAFT, preset: 'custom', customSchedule: '0 9 * * 1-5' }, + customScheduleInvalid: false, + validateAdvancedSchedule: isValidAutomationCronSchedule, + onUseSimpleSchedule: () => undefined, + onDraftChange: () => undefined + }) + ) + + expect(markup).toContain('Quick starts') + expect(markup).toContain('Every 15 min') + expect(markup).toContain('Cron expression') + expect(markup).toContain('Minute') + expect(markup).toContain('Weekday') + expect(markup).toContain('automation-cron-status') + }) +}) diff --git a/src/renderer/src/components/automations/AutomationSchedulePicker.tsx b/src/renderer/src/components/automations/AutomationSchedulePicker.tsx index 7e5c72f19..b45bce35c 100644 --- a/src/renderer/src/components/automations/AutomationSchedulePicker.tsx +++ b/src/renderer/src/components/automations/AutomationSchedulePicker.tsx @@ -1,7 +1,6 @@ import React from 'react' import { CalendarClock, ChevronsUpDown } from 'lucide-react' import { Button } from '@/components/ui/button' -import { Input } from '@/components/ui/input' import { Popover, PopoverContent, PopoverTrigger } from '@/components/ui/popover' import { cn } from '@/lib/utils' import { @@ -13,23 +12,25 @@ import { } from '@/components/ui/select' import type { AutomationSchedulePreset } from '../../../../shared/automations-types' import { + buildAutomationCronSchedule, buildAutomationRrule, classifyAutomationCronSchedule, formatAutomationSchedule, isValidAutomationSchedule } from '../../../../shared/automation-schedules' import type { AutomationDraft } from './AutomationEditorDialog' +import { AutomationCustomCronPanel } from './AutomationCustomCronPanel' import { Field } from './automation-page-parts' const FIELD_CONTROL_CLASS = 'border-input bg-input/30 shadow-xs dark:bg-input/30' -type SimpleSchedulePreset = Exclude -const SIMPLE_PRESETS = [ +export const AUTOMATION_SCHEDULE_PRESET_OPTIONS = [ ['hourly', 'Hourly'], ['daily', 'Daily'], ['weekdays', 'Weekdays'], - ['weekly', 'Weekly'] -] as const + ['weekly', 'Weekly'], + ['custom', 'Custom cron'] +] as const satisfies readonly [AutomationSchedulePreset, string][] const DAY_OPTIONS = [ ['0', 'Sunday'], @@ -130,6 +131,34 @@ function getSimpleScheduleDraft( return { preset: 'weekdays', time: current.time, dayOfWeek: current.dayOfWeek || '1' } } +function buildCustomScheduleSeed(draft: AutomationDraft): string { + const existing = draft.customSchedule.trim() + if (existing) { + return draft.customSchedule + } + if (draft.preset === 'custom') { + return '' + } + const { hour, minute } = parseTime(draft.time) + return buildAutomationCronSchedule({ + preset: draft.preset, + hour, + minute, + dayOfWeek: Number(draft.dayOfWeek) + }) +} + +export function getSchedulePresetDraft( + current: AutomationDraft, + preset: AutomationSchedulePreset +): Pick { + return { + preset, + customSchedule: preset === 'custom' ? buildCustomScheduleSeed(current) : current.customSchedule, + scheduleWarning: null + } +} + export function AutomationSchedulePicker({ draft, triggerClassName, @@ -169,51 +198,23 @@ export function AutomationSchedulePicker({
{draft.preset === 'custom' ? ( -
- - - onDraftChange((current) => ({ - ...current, - customSchedule: event.target.value, - scheduleWarning: null - })) - } - /> -
- Existing advanced schedules are preserved until you choose a simple schedule. -
- {customScheduleInvalid ? ( -
- Enter a valid advanced schedule before saving. -
- ) : null} - - -
+ + onDraftChange((current) => ({ + ...current, + ...getSimpleScheduleDraft(current), + scheduleWarning: null + })) + } + /> ) : ( <> @@ -222,8 +223,7 @@ export function AutomationSchedulePicker({ onValueChange={(preset) => onDraftChange((current) => ({ ...current, - preset: preset as SimpleSchedulePreset, - scheduleWarning: null + ...getSchedulePresetDraft(current, preset as AutomationSchedulePreset) })) } > @@ -231,7 +231,7 @@ export function AutomationSchedulePicker({ - {SIMPLE_PRESETS.map(([value, presetLabel]) => ( + {AUTOMATION_SCHEDULE_PRESET_OPTIONS.map(([value, presetLabel]) => ( {presetLabel} diff --git a/src/renderer/src/components/automations/AutomationsPage.tsx b/src/renderer/src/components/automations/AutomationsPage.tsx index 8aee616f3..bc3a58b20 100644 --- a/src/renderer/src/components/automations/AutomationsPage.tsx +++ b/src/renderer/src/components/automations/AutomationsPage.tsx @@ -54,6 +54,7 @@ import type { SshConnectionStatus } from '../../../../shared/ssh-types' import type { Worktree } from '../../../../shared/types' import { getWorktreePathBasenameFromId } from '../../../../shared/worktree-id' import { + buildAutomationCronSchedule, buildAutomationRrule, formatAutomationSchedule, isValidAutomationCronSchedule, @@ -136,16 +137,12 @@ function buildHermesCronSchedule(draft: AutomationDraft): string { return draft.customSchedule.trim() } const { hour, minute } = parseDraftTime(draft.time) - if (draft.preset === 'hourly') { - return `${minute} * * * *` - } - if (draft.preset === 'daily') { - return `${minute} ${hour} * * *` - } - if (draft.preset === 'weekdays') { - return `${minute} ${hour} * * 1-5` - } - return `${minute} ${hour} * * ${Number(draft.dayOfWeek)}` + return buildAutomationCronSchedule({ + preset: draft.preset, + hour, + minute, + dayOfWeek: Number(draft.dayOfWeek) + }) } function getAgentLabel(agentId: string): string { diff --git a/src/shared/automation-schedules.test.ts b/src/shared/automation-schedules.test.ts index 027c59fe5..f6f269a00 100644 --- a/src/shared/automation-schedules.test.ts +++ b/src/shared/automation-schedules.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from 'vitest' import { + buildAutomationCronSchedule, buildAutomationRrule, classifyAutomationCronSchedule, formatAutomationSchedule, @@ -94,6 +95,19 @@ describe('automation schedules', () => { expect(latest).toBe(new Date('2026-05-15T10:15:00').getTime()) }) + it('builds cron schedules from simple automation presets', () => { + expect(buildAutomationCronSchedule({ preset: 'hourly', hour: 9, minute: 15 })).toBe( + '15 * * * *' + ) + expect(buildAutomationCronSchedule({ preset: 'daily', hour: 9, minute: 15 })).toBe('15 9 * * *') + expect(buildAutomationCronSchedule({ preset: 'weekdays', hour: 9, minute: 15 })).toBe( + '15 9 * * 1-5' + ) + expect( + buildAutomationCronSchedule({ preset: 'weekly', hour: 9, minute: 15, dayOfWeek: 0 }) + ).toBe('15 9 * * 0') + }) + it('formats simple cron schedules with friendly labels', () => { expect(formatAutomationSchedule('5 * * * *')).toBe('Hourly at :05') expect(formatAutomationSchedule('15 10 * * *')).toBe(`Daily at ${formatTimeForTest(10, 15)}`) diff --git a/src/shared/automation-schedules.ts b/src/shared/automation-schedules.ts index 7eb90c352..5a477163d 100644 --- a/src/shared/automation-schedules.ts +++ b/src/shared/automation-schedules.ts @@ -490,6 +490,27 @@ export function buildAutomationRrule(args: { return `FREQ=DAILY;BYHOUR=${hour};BYMINUTE=${minute}` } +export function buildAutomationCronSchedule(args: { + preset: Exclude + hour: number + minute: number + dayOfWeek?: number +}): string { + const hour = Math.max(0, Math.min(23, Math.floor(args.hour))) + const minute = Math.max(0, Math.min(59, Math.floor(args.minute))) + if (args.preset === 'hourly') { + return `${minute} * * * *` + } + if (args.preset === 'weekdays') { + return `${minute} ${hour} * * 1-5` + } + if (args.preset === 'weekly') { + const day = Math.max(0, Math.min(6, Math.floor(args.dayOfWeek ?? 1))) + return `${minute} ${hour} * * ${day}` + } + return `${minute} ${hour} * * *` +} + export function nextAutomationOccurrenceAfter( rrule: string, dtstart: number,