fix: respect defaultPeriod and allowedPeriods in period detection (#87)

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: xrutayisire

.claude/skills/visual-test/SKILL.md

@@ -15,11 +15,16 @@ Compare the local Storybook running on this branch against the production deploy
 
 ## Setup
 
-1. Check if local Storybook is already running on port 9009. If not, start it:
+1. Create (or empty) the screenshots directory at the repository root:
+   ```bash
+   rm -rf <project-root>/screenshots && mkdir -p <project-root>/screenshots
+   ```
+   All screenshots taken during this test must be saved inside `<project-root>/screenshots/`.
+2. Check if local Storybook is already running on port 9009. If not, start it:
    ```bash
    cd <project-root> && yarn storybook &
    ```
-2. Wait for `http://localhost:9009` to respond before proceeding.
+3. Wait for `http://localhost:9009` to respond before proceeding.
 
 ## Test procedure
 
@@ -28,10 +33,10 @@ For **every story** listed below, perform the full comparison cycle:
 ### Comparison cycle (repeat for each story)
 
 1. **Navigate to the production story** at the given URL.
-2. **Take a full-page screenshot** and save it as `prod-<story-slug>.png`.
+2. **Take a full-page screenshot** and save it as `screenshots/prod-<story-slug>.png`.
 3. **Take an accessibility snapshot** of the production page.
 4. **Navigate to the same story locally** at `http://localhost:9009`.
-5. **Take a full-page screenshot** and save it as `local-<story-slug>.png`.
+5. **Take a full-page screenshot** and save it as `screenshots/local-<story-slug>.png`.
 6. **Take an accessibility snapshot** of the local page.
 7. **Compare both screenshots visually.** Report any difference in:
    - Text content (every label, placeholder, value, heading, and button text must be identical)
@@ -85,7 +90,7 @@ Test each of the following stories by navigating to the corresponding Storybook
 
 ## Interaction tests
 
-After the static visual comparison of all stories, perform the following interaction tests on the **Dynamic Settings** story. For each interaction, perform the action on both production and local, then compare screenshots and snapshots.
+After the static visual comparison of all stories, perform the following interaction tests on the **Dynamic Settings** story. For each interaction, perform the action on both production and local, then compare screenshots and snapshots. Save all interaction screenshots in the `screenshots/` folder using the naming pattern `screenshots/prod-<interaction-slug>.png` and `screenshots/local-<interaction-slug>.png`.
 
 ### Period dropdown
 
@@ -193,3 +198,11 @@ After all checks are complete, provide a summary:
 - Pass/fail verdict
 
 If all checks pass, confirm: **"No visual, textual, or interaction regressions detected. The branch is safe to merge."**
+
+## Cleanup
+
+After the final report is delivered, delete all contents of the screenshots folder:
+```bash
+rm -rf <project-root>/screenshots
+```
+This ensures no screenshot artifacts remain in the repository after the test run.

src/Cron.tsx

@@ -23,7 +23,7 @@ export default function Cron(props: CronProps) {
     displayError = true,
     onError,
     className,
-    defaultPeriod = 'day',
+    defaultPeriod,
     allowEmpty = 'for-default-value',
     humanizeLabels = true,
     humanizeValue = false,
@@ -64,7 +64,8 @@ export default function Cron(props: CronProps) {
     getPopupContainer,
   } = props
   const internalValueRef = useRef<string>(value)
-  const defaultPeriodRef = useRef<PeriodType>(defaultPeriod)
+  const effectiveDefaultPeriod = defaultPeriod ?? 'day'
+  const defaultPeriodRef = useRef<PeriodType>(effectiveDefaultPeriod)
   const [period, setPeriod] = useState<PeriodType | undefined>()
   const [monthDays, setMonthDays] = useState<number[] | undefined>()
   const [months, setMonths] = useState<number[] | undefined>()
@@ -94,6 +95,8 @@ export default function Cron(props: CronProps) {
         setMonths,
         setWeekDays,
         setPeriod,
+        defaultPeriod,
+        allowedPeriods,
       )
     },
     // eslint-disable-next-line react-hooks/exhaustive-deps
@@ -118,6 +121,8 @@ export default function Cron(props: CronProps) {
           setMonths,
           setWeekDays,
           setPeriod,
+          defaultPeriod,
+          allowedPeriods,
         )
       }
     },

src/converter.ts

@@ -36,6 +36,8 @@ export function setValuesFromCronString(
   setMonths: SetValueNumbersOrUndefined,
   setWeekDays: SetValueNumbersOrUndefined,
   setPeriod: SetValuePeriod,
+  defaultPeriod?: PeriodType,
+  allowedPeriods?: PeriodType[],
 ) {
   if (onError) {
     onError(undefined)
@@ -80,7 +82,11 @@ export function setValuesFromCronString(
 
     try {
       const cronParts = parseCronString(cronString)
-      const period = getPeriodFromCronParts(cronParts)
+      const period = getPeriodFromCronParts(
+        cronParts,
+        defaultPeriod,
+        allowedPeriods,
+      )
 
       setPeriod(period)
       setMinutes(cronParts[0])
@@ -285,21 +291,75 @@ function cronToString(parts: string[]) {
 }
 
 /**
- * Find the period from cron parts
+ * Period hierarchy from most specific to most broad.
+ * A cron expression with minimum period P is compatible with any period >= P.
+ */
+const PERIOD_ORDER: PeriodType[] = [
+  'minute',
+  'hour',
+  'day',
+  'week',
+  'month',
+  'year',
+]
+
+function getPeriodRank(period: PeriodType): number {
+  const rank = PERIOD_ORDER.indexOf(period)
+  return rank === -1 ? -1 : rank
+}
+
+/**
+ * Find the period from cron parts, respecting defaultPeriod and allowedPeriods
+ * when the cron expression is ambiguous (compatible with multiple periods).
  */
-function getPeriodFromCronParts(cronParts: number[][]): PeriodType {
+function getPeriodFromCronParts(
+  cronParts: number[][],
+  defaultPeriod?: PeriodType,
+  allowedPeriods?: PeriodType[],
+): PeriodType {
+  // Determine the minimum compatible period from the cron fields
+  let minPeriod: PeriodType
   if (cronParts[3].length > 0) {
-    return 'year'
+    minPeriod = 'year'
   } else if (cronParts[2].length > 0) {
-    return 'month'
+    minPeriod = 'month'
   } else if (cronParts[4].length > 0) {
-    return 'week'
+    minPeriod = 'week'
   } else if (cronParts[1].length > 0) {
-    return 'day'
+    minPeriod = 'day'
   } else if (cronParts[0].length > 0) {
-    return 'hour'
+    minPeriod = 'hour'
+  } else {
+    minPeriod = 'minute'
   }
-  return 'minute'
+
+  const minRank = getPeriodRank(minPeriod)
+
+  // If defaultPeriod is explicitly provided and compatible, prefer it
+  if (defaultPeriod) {
+    const defaultRank = getPeriodRank(defaultPeriod)
+
+    if (
+      defaultRank >= minRank &&
+      (!allowedPeriods || allowedPeriods.includes(defaultPeriod))
+    ) {
+      return defaultPeriod
+    }
+  }
+
+  // Find the smallest allowed period that is compatible (rank >= minRank)
+  if (allowedPeriods) {
+    for (let i = minRank; i < PERIOD_ORDER.length; i++) {
+      if (allowedPeriods.includes(PERIOD_ORDER[i])) {
+        return PERIOD_ORDER[i]
+      }
+    }
+  } else {
+    return minPeriod
+  }
+
+  // Fallback: use minPeriod even if not allowed (edge case)
+  return minPeriod
 }
 
 /**

src/tests/Cron.defaultValue.test.tsx

@@ -31,6 +31,7 @@ describe('Cron defaultValue test suite', () => {
     clockFormat?: ClockFormat
     allowedDropdowns?: CronType[]
     defaultPeriod?: PeriodType
+    allowedPeriods?: PeriodType[]
     periodSelect: PeriodType | undefined
     monthsSelect: string | undefined
     monthDaysSelect: string | undefined
@@ -248,15 +249,52 @@ describe('Cron defaultValue test suite', () => {
       minutesSelect: 'every minute',
     },
     {
-      title: 'that default period is ignored when default value is not empty',
+      title:
+        'that default period is respected when value is compatible with it',
       defaultValue: '* * * * *',
       defaultPeriod: 'year',
-      periodSelect: 'minute',
+      periodSelect: 'year',
+      monthsSelect: 'every month',
+      monthDaysSelect: 'every day of the month',
+      weekDaysSelect: 'every day of the week',
+      hoursSelect: 'every hour',
+      minutesSelect: 'every minute',
+    },
+    {
+      title:
+        'that default period week is respected for ambiguous daily cron expression',
+      defaultValue: '10 10 * * *',
+      defaultPeriod: 'week',
+      periodSelect: 'week',
       monthsSelect: undefined,
       monthDaysSelect: undefined,
-      weekDaysSelect: undefined,
-      hoursSelect: undefined,
-      minutesSelect: undefined,
+      weekDaysSelect: 'every day of the week',
+      hoursSelect: '10',
+      minutesSelect: '10',
+    },
+    {
+      title:
+        'that default period is ignored when value is not compatible with it',
+      defaultValue: '10 10 * * 1',
+      defaultPeriod: 'day',
+      periodSelect: 'week',
+      monthsSelect: undefined,
+      monthDaysSelect: undefined,
+      weekDaysSelect: 'MON',
+      hoursSelect: '10',
+      minutesSelect: '10',
+    },
+    {
+      title:
+        'that allowedPeriods is respected when detected period is not allowed',
+      defaultValue: '10 10 * * *',
+      allowedPeriods: ['week', 'month', 'year'],
+      periodSelect: 'week',
+      monthsSelect: undefined,
+      monthDaysSelect: undefined,
+      weekDaysSelect: 'every day of the week',
+      hoursSelect: '10',
+      minutesSelect: '10',
     },
     {
       title:
@@ -746,6 +784,7 @@ describe('Cron defaultValue test suite', () => {
       clockFormat,
       allowedDropdowns,
       defaultPeriod,
+      allowedPeriods,
       periodSelect,
       monthsSelect,
       monthDaysSelect,
@@ -771,6 +810,7 @@ describe('Cron defaultValue test suite', () => {
           clockFormat={clockFormat}
           allowedDropdowns={allowedDropdowns}
           defaultPeriod={defaultPeriod}
+          allowedPeriods={allowedPeriods}
           dropdownsConfig={dropdownsConfig}
         />,
       )

src/types.ts

@@ -48,7 +48,9 @@ export interface CronProps {
   leadingZero?: LeadingZero
 
   /**
-   * Define the default period when the default value is empty.
+   * Define the default period when the value is empty.
+   * When set and the cron value is ambiguous (compatible with multiple periods),
+   * this period will be preferred over the auto-detected one.
    *
    * Default: 'day'
    */