fix: address review findings (#3983)
This commit is contained in:
Jinjing
GitHub
parent
a79ea461c3
commit
0c751f46a3
|
|
@ -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 (
|
||||
<div className="grid gap-3">
|
||||
<div className="rounded-md border border-border/70 bg-muted/25 p-2.5">
|
||||
<div className="mb-2 flex items-center justify-between gap-2">
|
||||
<div className="text-xs font-medium">Quick starts</div>
|
||||
<Button
|
||||
type="button"
|
||||
variant="ghost"
|
||||
size="xs"
|
||||
className="h-6 px-1.5 text-muted-foreground hover:text-foreground"
|
||||
onClick={onUseSimpleSchedule}
|
||||
>
|
||||
<RotateCcw className="size-3.5" />
|
||||
Simple
|
||||
</Button>
|
||||
</div>
|
||||
<div className="grid grid-cols-2 gap-2">
|
||||
{AUTOMATION_CRON_QUICK_STARTS.map((preset) => (
|
||||
<Button
|
||||
key={preset.expression}
|
||||
type="button"
|
||||
variant="outline"
|
||||
size="sm"
|
||||
className="h-auto min-h-11 flex-col items-start gap-0.5 px-2 py-1.5 text-left"
|
||||
onClick={() =>
|
||||
onDraftChange((current) => ({
|
||||
...current,
|
||||
customSchedule: preset.expression,
|
||||
scheduleWarning: null
|
||||
}))
|
||||
}
|
||||
>
|
||||
<span className="text-xs font-medium">{preset.label}</span>
|
||||
<span className="font-mono text-[11px] text-muted-foreground">
|
||||
{preset.expression}
|
||||
</span>
|
||||
</Button>
|
||||
))}
|
||||
</div>
|
||||
</div>
|
||||
<Field label="Cron expression">
|
||||
<Input
|
||||
value={draft.customSchedule}
|
||||
placeholder="0 9 * * 1-5"
|
||||
spellCheck={false}
|
||||
className={cn('font-mono', FIELD_CONTROL_CLASS)}
|
||||
aria-invalid={customScheduleInvalid}
|
||||
aria-describedby="automation-cron-status"
|
||||
onChange={(event) =>
|
||||
onDraftChange((current) => ({
|
||||
...current,
|
||||
customSchedule: event.target.value,
|
||||
scheduleWarning: null
|
||||
}))
|
||||
}
|
||||
/>
|
||||
<div className="mt-2 grid grid-cols-5 gap-1.5">
|
||||
{AUTOMATION_CRON_FIELD_LABELS.map((label, index) => (
|
||||
<div
|
||||
key={label}
|
||||
className="min-w-0 rounded-md border border-border/70 bg-muted/25 px-1.5 py-1 text-center"
|
||||
>
|
||||
<div className="truncate text-[10px] font-medium text-muted-foreground">{label}</div>
|
||||
<div className="mt-0.5 truncate font-mono text-[11px] text-foreground">
|
||||
{cronFieldValues[index]}
|
||||
</div>
|
||||
</div>
|
||||
))}
|
||||
</div>
|
||||
<div
|
||||
id="automation-cron-status"
|
||||
className={cn(
|
||||
'mt-2 flex min-h-8 items-center gap-2 rounded-md border px-2 py-1.5 text-xs',
|
||||
customScheduleStatus.kind === 'invalid'
|
||||
? 'border-destructive/40 bg-destructive/10 text-destructive'
|
||||
: 'border-border/70 bg-muted/30 text-muted-foreground'
|
||||
)}
|
||||
>
|
||||
{customScheduleStatus.kind === 'invalid' ? (
|
||||
<CircleAlert className="size-3.5 shrink-0" />
|
||||
) : (
|
||||
<CheckCircle2 className="size-3.5 shrink-0" />
|
||||
)}
|
||||
<span className="min-w-0 truncate">{customScheduleStatus.label}</span>
|
||||
</div>
|
||||
</Field>
|
||||
</div>
|
||||
)
|
||||
}
|
||||
|
|
@ -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')
|
||||
})
|
||||
})
|
||||
|
|
@ -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<AutomationSchedulePreset, 'custom'>
|
||||
|
||||
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<AutomationDraft, 'preset' | 'customSchedule' | 'scheduleWarning'> {
|
||||
return {
|
||||
preset,
|
||||
customSchedule: preset === 'custom' ? buildCustomScheduleSeed(current) : current.customSchedule,
|
||||
scheduleWarning: null
|
||||
}
|
||||
}
|
||||
|
||||
export function AutomationSchedulePicker({
|
||||
draft,
|
||||
triggerClassName,
|
||||
|
|
@ -169,51 +198,23 @@ export function AutomationSchedulePicker({
|
|||
</PopoverTrigger>
|
||||
<PopoverContent
|
||||
align="start"
|
||||
className="w-[min(var(--radix-popover-trigger-width),calc(100vw-2rem))] min-w-[min(18rem,calc(100vw-2rem))] max-w-[calc(100vw-2rem)] p-3"
|
||||
className="w-[min(var(--radix-popover-trigger-width),calc(100vw-2rem))] min-w-[min(22rem,calc(100vw-2rem))] max-w-[calc(100vw-2rem)] p-3"
|
||||
>
|
||||
<div className="grid gap-3">
|
||||
{draft.preset === 'custom' ? (
|
||||
<div className="grid gap-3">
|
||||
<Field label="Advanced schedule">
|
||||
<Input
|
||||
value={draft.customSchedule}
|
||||
placeholder="0 9 * * 1-5"
|
||||
spellCheck={false}
|
||||
className={cn('font-mono', FIELD_CONTROL_CLASS)}
|
||||
aria-invalid={customScheduleInvalid}
|
||||
onChange={(event) =>
|
||||
onDraftChange((current) => ({
|
||||
...current,
|
||||
customSchedule: event.target.value,
|
||||
scheduleWarning: null
|
||||
}))
|
||||
}
|
||||
/>
|
||||
<div className="mt-1 text-[11px] text-muted-foreground">
|
||||
Existing advanced schedules are preserved until you choose a simple schedule.
|
||||
</div>
|
||||
{customScheduleInvalid ? (
|
||||
<div className="mt-1 text-[11px] text-destructive">
|
||||
Enter a valid advanced schedule before saving.
|
||||
</div>
|
||||
) : null}
|
||||
</Field>
|
||||
<Button
|
||||
type="button"
|
||||
variant="secondary"
|
||||
size="sm"
|
||||
className="justify-start"
|
||||
onClick={() =>
|
||||
onDraftChange((current) => ({
|
||||
...current,
|
||||
...getSimpleScheduleDraft(current),
|
||||
scheduleWarning: null
|
||||
}))
|
||||
}
|
||||
>
|
||||
Use simple schedule
|
||||
</Button>
|
||||
</div>
|
||||
<AutomationCustomCronPanel
|
||||
draft={draft}
|
||||
customScheduleInvalid={customScheduleInvalid}
|
||||
validateAdvancedSchedule={validateAdvancedSchedule}
|
||||
onDraftChange={onDraftChange}
|
||||
onUseSimpleSchedule={() =>
|
||||
onDraftChange((current) => ({
|
||||
...current,
|
||||
...getSimpleScheduleDraft(current),
|
||||
scheduleWarning: null
|
||||
}))
|
||||
}
|
||||
/>
|
||||
) : (
|
||||
<>
|
||||
<Field label="Cadence">
|
||||
|
|
@ -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({
|
|||
<SelectValue />
|
||||
</SelectTrigger>
|
||||
<SelectContent>
|
||||
{SIMPLE_PRESETS.map(([value, presetLabel]) => (
|
||||
{AUTOMATION_SCHEDULE_PRESET_OPTIONS.map(([value, presetLabel]) => (
|
||||
<SelectItem key={value} value={value}>
|
||||
{presetLabel}
|
||||
</SelectItem>
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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)}`)
|
||||
|
|
|
|||
|
|
@ -490,6 +490,27 @@ export function buildAutomationRrule(args: {
|
|||
return `FREQ=DAILY;BYHOUR=${hour};BYMINUTE=${minute}`
|
||||
}
|
||||
|
||||
export function buildAutomationCronSchedule(args: {
|
||||
preset: Exclude<AutomationSchedulePreset, 'custom'>
|
||||
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,
|
||||
|
|
|
|||
Loading…
Reference in New Issue