feat(schedules): add monthly weekday schedules (Nth weekday and offset) - #4116
Open
makaiver wants to merge 7 commits into
Open
feat(schedules): add monthly weekday schedules (Nth weekday and offset)#4116makaiver wants to merge 7 commits into
makaiver wants to merge 7 commits into
Conversation
- Add "@monthly-weekday <ordinal> <weekday> [offset <days>] [at <HH:MM>]" cron descriptor, e.g. first Wednesday after Patch Tuesday - Custom cron.ScheduleParser backs both the pool and ValidateCronFormat and delegates every other expression to the standard parser - Reuse cron_format (no migration); HA-safe; no new dependency - Return next_run from the validate endpoint for the UI preview - Add "Monthly (by weekday)" builder to ScheduleForm, matching the Monthly design
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded monthly-weekday cron descriptors with ordinal, weekday, offset, time, and timezone support. Added backend validation previews, frontend scheduling controls, parser integration, and behavior and compatibility tests. ChangesMonthly weekday scheduling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ScheduleForm
participant ValidateScheduleCronFormat
participant NextRunTimes
participant newCronParser
ScheduleForm->>ValidateScheduleCronFormat: submit monthly-weekday cron format
ValidateScheduleCronFormat->>NextRunTimes: request five upcoming runs
NextRunTimes->>newCronParser: parse schedule descriptor
newCronParser-->>NextRunTimes: parsed schedule
NextRunTimes-->>ValidateScheduleCronFormat: next_run timestamps
ValidateScheduleCronFormat-->>ScheduleForm: validation response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/ScheduleForm.vue (1)
720-740: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDiscard stale validation responses before writing
backendNextRuns.
refreshCron()callsvalidateCronFormat()without awaiting it, and the offset field triggers one call per keystroke. Two in-flight requests can resolve out of order. The later-resolving older response then overwritesbackendNextRuns, and the "Next run time" preview shows runs for a descriptor the user already replaced. The preview does not self-correct until the next edit.
refreshCheckboxes()already guards against this at Line 825. Apply the same guard insidevalidateCronFormat()so every caller benefits.🐛 Proposed staleness guard
async validateCronFormat(cronFormat) { try { const res = await axios({ method: 'post', url: `/api/project/${this.projectId}/schedules/validate`, responseType: 'json', data: { project_id: this.projectId, cron_format: cronFormat, }, }); + if (cronFormat !== this.item.cron_format) { + return null; // the value changed while validating, ignore stale result + } const runs = (res.data && res.data.next_run) || []; this.backendNextRuns = runs .map((s) => new Date(s)) .filter((d) => !Number.isNaN(d.getTime())); return null; } catch (err) { + if (cronFormat !== this.item.cron_format) { + return null; + } this.backendNextRuns = []; return getErrorMessage(err); } },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/ScheduleForm.vue` around lines 720 - 740, Update validateCronFormat to use the same request-generation or staleness guard pattern as refreshCheckboxes before assigning backendNextRuns, including the error path that clears it. Ensure only the latest validation request may update backendNextRuns, while preserving the existing successful parsing and error-message behavior.
🧹 Nitpick comments (2)
services/schedules/monthly_weekday_regression_test.go (1)
38-81: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd a short-mode escape to the new exhaustive sweeps in
services/schedules. Three new tests in this package run large generated sweeps with notesting.Short()guard, so every CI run pays the full cost. The shared fix is one skip guard per sweep.
services/schedules/monthly_weekday_regression_test.go#L38-L81: addif testing.Short() { t.Skip(...) }at the start ofTestDiffSweep_NoRegressionVsParseStandard, which generates 23,520 specs and parses each one twice.services/schedules/monthly_weekday_property_test.go#L110-L141: add the same guard toTestMonthlyWeekday_Prop_BruteForceOracle, which compares 378 schedules against 12 reference instants with a day-by-day oracle.services/schedules/monthly_weekday_property_test.go#L146-L167: add the same guard toTestMonthlyWeekday_Prop_OneMonthBackSufficiency, which steps every 6 hours across two years for each of 5 schedules.Keep the full sweeps for the normal
go testrun.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/schedules/monthly_weekday_regression_test.go` around lines 38 - 81, Add an early testing.Short() skip guard to TestDiffSweep_NoRegressionVsParseStandard in services/schedules/monthly_weekday_regression_test.go:38-81, TestMonthlyWeekday_Prop_BruteForceOracle in services/schedules/monthly_weekday_property_test.go:110-141, and TestMonthlyWeekday_Prop_OneMonthBackSufficiency in services/schedules/monthly_weekday_property_test.go:146-167. Keep the complete generated sweeps running during normal go test execution.web/src/components/ScheduleForm.vue (1)
234-244: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound the offset input and debounce the validation request.
The
@inputhandler callsrefreshCron()on every keystroke, andrefreshCron()posts to/schedules/validatefor the monthly-weekday timing. Typing a two-character offset such as-5sends two requests, and one of them is for the intermediate value-. Addmin="-28"andmax="28"to match the backend limit of ±28 days, and debounce the handler or switch to@change.♻️ Proposed change to the offset field
<v-text-field v-model.number="mwOffset" type="number" + min="-28" + max="28" label="Offset (days)" hint="e.g. Second Tuesday + 1 = first Wednesday after Patch Tuesday" persistent-hint :disabled="formSaving" - `@input`="refreshCron()" + `@change`="refreshCron()" outlined dense />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/ScheduleForm.vue` around lines 234 - 244, Update the mwOffset v-text-field to enforce the backend range with min="-28" and max="28", and stop calling refreshCron() for every keystroke by debouncing the handler or switching it to `@change` so intermediate values such as "-" do not trigger validation requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@services/schedules/monthly_weekday_property_test.go`:
- Around line 43-46: Update the doc comment for oracleNext to describe the
actual enumeration window implemented by its month-2 start and 12-month
iteration: t-2 months through t+9 months. Leave the code and the inline comment
unchanged.
In `@web/src/components/ScheduleForm.vue`:
- Around line 952-959: Update refreshCron() in the monthlyWeekday branch to
assign the return value of validateCronFormat(this.item.cron_format) to
this.cronFormatError, matching the existing handling in refreshCheckboxes().
Preserve the backend-authoritative preview refresh and early return behavior.
---
Outside diff comments:
In `@web/src/components/ScheduleForm.vue`:
- Around line 720-740: Update validateCronFormat to use the same
request-generation or staleness guard pattern as refreshCheckboxes before
assigning backendNextRuns, including the error path that clears it. Ensure only
the latest validation request may update backendNextRuns, while preserving the
existing successful parsing and error-message behavior.
---
Nitpick comments:
In `@services/schedules/monthly_weekday_regression_test.go`:
- Around line 38-81: Add an early testing.Short() skip guard to
TestDiffSweep_NoRegressionVsParseStandard in
services/schedules/monthly_weekday_regression_test.go:38-81,
TestMonthlyWeekday_Prop_BruteForceOracle in
services/schedules/monthly_weekday_property_test.go:110-141, and
TestMonthlyWeekday_Prop_OneMonthBackSufficiency in
services/schedules/monthly_weekday_property_test.go:146-167. Keep the complete
generated sweeps running during normal go test execution.
In `@web/src/components/ScheduleForm.vue`:
- Around line 234-244: Update the mwOffset v-text-field to enforce the backend
range with min="-28" and max="28", and stop calling refreshCron() for every
keystroke by debouncing the handler or switching it to `@change` so intermediate
values such as "-" do not trigger validation requests.
🪄 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 Plus
Run ID: 46a630cd-c6e6-4d17-976b-6d8b27ca0e7b
📒 Files selected for processing (10)
AGENTS/plans/2_20/schedule-monthly-weekday.mdapi-docs.ymlapi/projects/schedules.goapi/projects/schedules_test.goservices/schedules/SchedulePool.goservices/schedules/monthly_weekday.goservices/schedules/monthly_weekday_property_test.goservices/schedules/monthly_weekday_regression_test.goservices/schedules/monthly_weekday_test.goweb/src/components/ScheduleForm.vue
…reviews - Guard validateCronFormat() against out-of-order responses: a stale reply no longer overwrites backendNextRuns or the error state - refreshCron() now surfaces a backend rejection (reachable through the free-input offset field) instead of silently blanking the preview; shown on the Offset field, since the raw-cron field is hidden in builder mode - Correct the oracle doc comment window (t-2 .. t+9 months)
Guard the three generated sweeps (differential vs ParseStandard, the brute-force oracle, and the two-year sufficiency check) with testing.Short() so `go test -short` skips them.
…lities Add doc comments to the two remaining parser helpers (parseOffset, parseHourMinute), the validate handler, and the test helpers whose purpose isn't obvious from the name (safeParse, descOf, fmtT, mustParseAmbient, setupScheduleConfig, postValidate) plus the two property tests with cryptic names. Self-describing test functions are left uncommented, matching the package style.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds first-class support for “Nth weekday of the month” scheduling (including an optional day offset) by introducing a new @monthly-weekday cron descriptor, wiring it into the backend scheduler/validator, and adding a corresponding UI builder + next-run preview support.
Changes:
- Backend: add a custom cron parser and schedule implementation for
@monthly-weekday …, and use it for both scheduling and validation. - API/UI: extend the schedules validate endpoint to return upcoming run times and use that to power the “Next run time” preview for descriptors the frontend cron parser can’t evaluate.
- Frontend: add a “Monthly (by weekday)” schedule builder that emits the descriptor into the existing
cron_formatfield.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| web/src/components/ScheduleForm.vue | Adds “Monthly (by weekday)” builder, descriptor parsing/formatting, and backend-powered preview fallback. |
| services/schedules/SchedulePool.go | Switches the cron pool and validation to a shared custom parser to keep “validates == fires”. |
| services/schedules/monthly_weekday.go | Implements @monthly-weekday parsing, schedule evaluation, and preview (NextRunTimes). |
| services/schedules/monthly_weekday_test.go | Unit tests for core monthly-weekday behavior, errors, timezone behavior, and preview. |
| services/schedules/monthly_weekday_regression_test.go | Differential tests to ensure no regressions vs cron.ParseStandard. |
| services/schedules/monthly_weekday_property_test.go | Property/oracle tests for correctness, DST behavior, and end-to-end pool parity. |
| api/projects/schedules.go | Extends /schedules/validate to return next_run on success. |
| api/projects/schedules_test.go | Tests validate endpoint behavior for descriptor, standard cron, and invalid inputs. |
| api-docs.yml | Documents the new descriptor and the validate endpoint response shape. |
| AGENTS/plans/2_20/schedule-monthly-weekday.md | Captures the implementation plan/design notes for the feature. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The applied validation guards dropped the function's closing "}," which broke parsing, and left the hour/minute check over the line-length limit. Add the brace back and split that check into two lines.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A suggestion on how to add support for scheduling tasks like "the third Wednesday of the month" or "the first Wednesday after Patch Tuesday," which standard cron can't express.
You write it as a simple schedule descriptor in the existing cron field:
@monthly-weekday [offset ] [at HH:MM]
Examples:
There's also a "Monthly (by weekday)" option in the schedule form - its Hours/Minutes use the same chip grids as the existing Monthly builder, plus Occurrence, Weekday, and an Offset (days) field - and the "Next run time" preview works for it. The offset makes patch-Tuesday-relative windows easy (Second Tuesday + 1 = the Wednesday after Patch Tuesday), which is handy when patching Windows servers.
Addresses #3600, #2349, #2241, #1172 - all requests for this (first Tuesday, third Friday, last Wednesday, first Monday of the month), which people have been working around with uggly hacks and external cron.
Notes
Summary by CodeRabbit