fix: address review findings (#3994)

This commit is contained in:
Jinjing 2026-05-30 23:24:18 -07:00 committed by GitHub
parent b235ff1e4d
commit b8944852de
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 29 additions and 125 deletions

View File

@ -1,6 +1,5 @@
import React from 'react'
import { CheckCircle2, CircleAlert, RotateCcw } from 'lucide-react'
import { Button } from '@/components/ui/button'
import { CheckCircle2, CircleAlert } from 'lucide-react'
import { Input } from '@/components/ui/input'
import { cn } from '@/lib/utils'
import { formatAutomationSchedule } from '../../../../shared/automation-schedules'
@ -9,13 +8,6 @@ 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(
@ -24,7 +16,7 @@ export function getCronScheduleStatusLabel(
): { 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.' }
return { kind: 'empty', label: 'Enter a five-field cron.' }
}
if (!validateSchedule(trimmed)) {
return { kind: 'invalid', label: 'Enter a valid five-field cron before saving.' }
@ -42,13 +34,11 @@ 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(
@ -59,44 +49,6 @@ export function AutomationCustomCronPanel({
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}

View File

@ -3,7 +3,6 @@ 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
@ -36,15 +35,6 @@ describe('AutomationSchedulePicker', () => {
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',
@ -65,7 +55,7 @@ describe('AutomationSchedulePicker', () => {
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.'
label: 'Enter a five-field cron.'
})
expect(getCronScheduleStatusLabel('not cron', isValidAutomationCronSchedule)).toEqual({
kind: 'invalid',
@ -81,19 +71,18 @@ describe('AutomationSchedulePicker', () => {
expect(getCronFieldValues('0 9')).toEqual(['0', '9', '...', '...', '...'])
})
it('renders quick starts beside the cron expression field', () => {
it('renders the cron expression field without quick starts', () => {
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).not.toContain('Quick starts')
expect(markup).not.toContain('Every 15 min')
expect(markup).toContain('Cron expression')
expect(markup).toContain('Minute')
expect(markup).toContain('Weekday')

View File

@ -14,7 +14,6 @@ import type { AutomationSchedulePreset } from '../../../../shared/automations-ty
import {
buildAutomationCronSchedule,
buildAutomationRrule,
classifyAutomationCronSchedule,
formatAutomationSchedule,
isValidAutomationSchedule
} from '../../../../shared/automation-schedules'
@ -102,35 +101,6 @@ function getDraftScheduleLabel(draft: AutomationDraft): string {
)
}
function getSimpleScheduleDraft(
current: AutomationDraft
): Pick<AutomationDraft, 'preset' | 'time' | 'dayOfWeek'> {
const classification = classifyAutomationCronSchedule(current.customSchedule)
if (classification.kind === 'hourly') {
const { hour } = parseTime(current.time)
return {
preset: 'hourly',
time: formatTimeInput(hour, classification.minute),
dayOfWeek: current.dayOfWeek
}
}
if (classification.kind === 'daily' || classification.kind === 'weekdays') {
return {
preset: classification.kind,
time: formatTimeInput(classification.hour, classification.minute),
dayOfWeek: current.dayOfWeek
}
}
if (classification.kind === 'weekly') {
return {
preset: 'weekly',
time: formatTimeInput(classification.hour, classification.minute),
dayOfWeek: String(classification.dayOfWeek)
}
}
return { preset: 'weekdays', time: current.time, dayOfWeek: current.dayOfWeek || '1' }
}
function buildCustomScheduleSeed(draft: AutomationDraft): string {
const existing = draft.customSchedule.trim()
if (existing) {
@ -198,47 +168,40 @@ export function AutomationSchedulePicker({
</PopoverTrigger>
<PopoverContent
align="start"
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"
className="popover-scroll-content scrollbar-sleek max-h-[var(--radix-popover-content-available-height)] w-[min(var(--radix-popover-trigger-width),calc(100vw-2rem))] min-w-[min(22rem,calc(100vw-2rem))] max-w-[calc(100vw-2rem)] overflow-y-auto p-3"
>
<div className="grid gap-3">
<Field label="Cadence">
<Select
value={draft.preset}
onValueChange={(preset) =>
onDraftChange((current) => ({
...current,
...getSchedulePresetDraft(current, preset as AutomationSchedulePreset)
}))
}
>
<SelectTrigger className={cn('w-full min-w-0', FIELD_CONTROL_CLASS)}>
<SelectValue />
</SelectTrigger>
<SelectContent>
{AUTOMATION_SCHEDULE_PRESET_OPTIONS.map(([value, presetLabel]) => (
<SelectItem key={value} value={value}>
{presetLabel}
</SelectItem>
))}
</SelectContent>
</Select>
</Field>
{draft.preset === 'custom' ? (
<AutomationCustomCronPanel
draft={draft}
customScheduleInvalid={customScheduleInvalid}
validateAdvancedSchedule={validateAdvancedSchedule}
onDraftChange={onDraftChange}
onUseSimpleSchedule={() =>
onDraftChange((current) => ({
...current,
...getSimpleScheduleDraft(current),
scheduleWarning: null
}))
}
/>
) : (
<>
<Field label="Cadence">
<Select
value={draft.preset}
onValueChange={(preset) =>
onDraftChange((current) => ({
...current,
...getSchedulePresetDraft(current, preset as AutomationSchedulePreset)
}))
}
>
<SelectTrigger className={cn('w-full min-w-0', FIELD_CONTROL_CLASS)}>
<SelectValue />
</SelectTrigger>
<SelectContent>
{AUTOMATION_SCHEDULE_PRESET_OPTIONS.map(([value, presetLabel]) => (
<SelectItem key={value} value={value}>
{presetLabel}
</SelectItem>
))}
</SelectContent>
</Select>
</Field>
{draft.preset === 'weekly' ? (
<Field label="Day">
<Select