feat: auto-update time window with configurable interval and UX improvements#2405
feat: auto-update time window with configurable interval and UX improvements#2405GiulioSavini wants to merge 16 commits intogetarcaneapp:mainfrom
Conversation
- auto-update window interval is now editable (was readonly */5 * * * * *) and persisted as autoUpdateWindowInterval setting; scheduler picks it up - run-now button stays in loading/spinner state for 3s after trigger so the user sees clear feedback (API returns instantly, job runs async) - save excluded containers before running auto-update job manually - schedule dialog highlights the currently active cron example with primary colour; pre-selects the current schedule on open
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
| case "autoUpdate", "autoUpdateInterval", | ||
| "autoUpdateWindowEnabled", "autoUpdateWindowStart", | ||
| "autoUpdateWindowEnd", "autoUpdateWindowDays": | ||
| changedAutoUpdate = true |
There was a problem hiding this comment.
Scheduler not rescheduled when
autoUpdateWindowInterval changes
autoUpdateWindowInterval is absent from the changedAutoUpdate case. When the user edits and saves the window interval, OnAutoUpdateSettingsChanged is never called, so the scheduler keeps its old cadence until a process restart — directly contradicting the PR test plan ("Change autoUpdateWindowInterval… save — verify scheduler picks up new cadence").
| case "autoUpdate", "autoUpdateInterval", | |
| "autoUpdateWindowEnabled", "autoUpdateWindowStart", | |
| "autoUpdateWindowEnd", "autoUpdateWindowDays": | |
| changedAutoUpdate = true | |
| case "autoUpdate", "autoUpdateInterval", | |
| "autoUpdateWindowEnabled", "autoUpdateWindowStart", | |
| "autoUpdateWindowEnd", "autoUpdateWindowDays", | |
| "autoUpdateWindowInterval": | |
| changedAutoUpdate = true |
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/internal/services/settings_service.go
Line: 759-762
Comment:
**Scheduler not rescheduled when `autoUpdateWindowInterval` changes**
`autoUpdateWindowInterval` is absent from the `changedAutoUpdate` case. When the user edits and saves the window interval, `OnAutoUpdateSettingsChanged` is never called, so the scheduler keeps its old cadence until a process restart — directly contradicting the PR test plan ("Change `autoUpdateWindowInterval`… save — verify scheduler picks up new cadence").
```suggestion
case "autoUpdate", "autoUpdateInterval",
"autoUpdateWindowEnabled", "autoUpdateWindowStart",
"autoUpdateWindowEnd", "autoUpdateWindowDays",
"autoUpdateWindowInterval":
changedAutoUpdate = true
```
How can I resolve this? If you propose a fix, please make it concise.| func (j *AutoUpdateJob) runAt(ctx context.Context, now time.Time) { | ||
| enabled := j.settingsService.GetBoolSetting(ctx, "autoUpdate", false) |
There was a problem hiding this comment.
Unexported functions missing
Internal suffix
Both runAt and isWithinWindow are unexported (lowercase) but don't carry the required Internal suffix per the project's naming convention. They should be renamed to runAtInternal and isWithinWindowInternal, and the test file updated accordingly.
| func (j *AutoUpdateJob) runAt(ctx context.Context, now time.Time) { | |
| enabled := j.settingsService.GetBoolSetting(ctx, "autoUpdate", false) | |
| func (j *AutoUpdateJob) runAtInternal(ctx context.Context, now time.Time) { |
Rule Used: What: All unexported functions must have the "Inte... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/pkg/scheduler/auto_update_job.go
Line: 67-68
Comment:
**Unexported functions missing `Internal` suffix**
Both `runAt` and `isWithinWindow` are unexported (lowercase) but don't carry the required `Internal` suffix per the project's naming convention. They should be renamed to `runAtInternal` and `isWithinWindowInternal`, and the test file updated accordingly.
```suggestion
func (j *AutoUpdateJob) runAtInternal(ctx context.Context, now time.Time) {
```
**Rule Used:** What: All unexported functions must have the "Inte... ([source](https://app.greptile.com/review/custom-context?memory=306fc233-4d2f-4ac4-bdf7-8059588e8a43))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| if (onBeforeRun) { | ||
| try { | ||
| await onBeforeRun(); | ||
| } catch (err) { | ||
| console.error('onBeforeRun failed:', err); | ||
| } | ||
| } |
There was a problem hiding this comment.
onBeforeRun failure is silently swallowed
If saving the excluded containers list fails, the error is only printed to the console and the job is still triggered with the old exclusion list. The user receives no toast notification, so they may believe their changes took effect when they did not.
Consider surfacing the error to the user before proceeding:
if (onBeforeRun) {
try {
await onBeforeRun();
} catch (err) {
console.error('onBeforeRun failed:', err);
toast.error(err instanceof Error ? err.message : m.jobs_run_failed());
return;
}
}Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/lib/components/job-card/job-card.svelte
Line: 65-71
Comment:
**`onBeforeRun` failure is silently swallowed**
If saving the excluded containers list fails, the error is only printed to the console and the job is still triggered with the old exclusion list. The user receives no toast notification, so they may believe their changes took effect when they did not.
Consider surfacing the error to the user before proceeding:
```ts
if (onBeforeRun) {
try {
await onBeforeRun();
} catch (err) {
console.error('onBeforeRun failed:', err);
toast.error(err instanceof Error ? err.message : m.jobs_run_failed());
return;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.| var autoUpdateWindowSettingKeys = []string{ | ||
| "autoUpdateWindowEnabled", | ||
| "autoUpdateWindowStart", | ||
| "autoUpdateWindowEnd", | ||
| "autoUpdateWindowDays", | ||
| "autoUpdateWindowInterval", | ||
| } |
There was a problem hiding this comment.
autoUpdateWindowSettingKeys is declared but never used
This slice is defined here but has no callers anywhere in the codebase. If it was intended for future use or as a helper for external consumers, it should be documented; otherwise it can be removed to avoid dead code.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/pkg/libarcane/settings.go
Line: 44-50
Comment:
**`autoUpdateWindowSettingKeys` is declared but never used**
This slice is defined here but has no callers anywhere in the codebase. If it was intended for future use or as a helper for external consumers, it should be documented; otherwise it can be removed to avoid dead code.
How can I resolve this? If you propose a fix, please make it concise.- add autoUpdateWindowInterval to changedAutoUpdate trigger so scheduler reschedules immediately when the window interval is saved - rename runAt/isWithinWindow to runAtInternal/isWithinWindowInternal per project naming convention; update test file accordingly - surface onBeforeRun failure as toast and abort the run instead of silently proceeding with stale data - remove unused autoUpdateWindowSettingKeys dead-code slice
Summary
autoUpdateWindowEnabled,autoUpdateWindowStart,autoUpdateWindowEnd,autoUpdateWindowDays) gate the auto-update job so it only runs within a configured time windowautoUpdateWindowInterval) is now a user-editable cron field (was hardcoded*/5 * * * * *)Test plan
autoUpdateWindowIntervalto e.g.0 */2 * * * *, save — verify scheduler picks up new cadence🤖 Generated with Claude Code
Disclaimer Greptiles Reviews use AI, make sure to check over its work.
To better help train Greptile on our codebase, if the comment is useful and valid Like the comment, if its not helpful or invalid Dislike
To have Greptile Re-Review the changes, mention
greptileai.Greptile Summary
This PR gates the auto-update job to a configurable time window (start/end time, days, check interval), adds a 3-second Run Now spinner with
onBeforeRunpre-save, highlights the active cron example in the schedule dialog, and blocks Arcane self-redeploy via compose.autoUpdateWindowIntervalmissing fromchangedAutoUpdateinsettings_service.go: changing only the window interval and saving will not reschedule the job — the old cadence persists until restart, directly breaking the PR's own test plan.autoUpdateWindowIntervalmissing fromcronSettingKeysinlibarcane/settings.go: no server-side cron validation is applied, so an invalid expression can be persisted and cause the scheduler to silently drop the auto-update job.Confidence Score: 3/5
Two P1 defects affecting the window-interval scheduling path should be fixed before merging.
Two independent P1 bugs exist: the scheduler is not rescheduled when
autoUpdateWindowIntervalchanges, and invalid cron expressions for that setting bypass server-side validation (potentially silently disabling auto-update). Both affect the primary new feature path and contradict the stated test plan.backend/internal/services/settings_service.go(missing case in changedAutoUpdate switch) andbackend/pkg/libarcane/settings.go(missing key in cronSettingKeys)Comments Outside Diff (1)
backend/pkg/libarcane/settings.go, line 35-42 (link)autoUpdateWindowIntervalis not validated as a cron expressionValidateCronSettingonly checks keys incronSettingKeys. BecauseautoUpdateWindowIntervalis missing from that list, an invalid cron string (e.g."not a cron") can be persisted without error.Schedule()then returns it verbatim to the cron scheduler, which will silently drop the job.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat: configurable window interval, run-..." | Re-trigger Greptile