[dev] [Marfuen] mariano/sale-49-custom-task-schedules#2675
Conversation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ntract Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…update service The BrowserAutomation.schedule column was dropped in the previous commit. TypeScript did not catch the dangling DTO entries or updateBrowserAutomation param because object spread does not trigger excess-property checks, but Prisma would throw "Unknown argument" at runtime if a caller supplied the field. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously, integrationLastRunAt was written whenever the check loop completed, even if one or more checks returned status='error' (meaning the check could not execute). On weekly+ schedules this would push the retry out a full period. Distinguish 'error' (infra issue, retry) from 'failed' (legitimate finding, ran successfully). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Updates test fixtures for Task schema additions (integrationScheduleFrequency, integrationLastRunAt, previousStatus, archivedAt, lastCompletedAt), adds TaskFrequency to the @db mock in browserbase.controller.spec so the new @IsEnum(TaskFrequency) DTO decorator resolves at module load, and fixes an undefined-index access in AutomationOverview when the scheduleFrequency field is missing on stale client types. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
4 issues found across 57 files
Note: This PR contains a large number of files. During the trial, cubic reviews up to 50 files per PR. Paid plans get a higher limit.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/TaskIntegrationChecks.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/TaskIntegrationChecks.tsx:320">
P2: The "Next run" calculation is hardcoded to daily at 6 AM UTC and ignores the newly added `scheduleFrequency` prop. When a user selects a non-daily schedule (e.g., weekly or monthly), the displayed "Next run" time will be misleading — it will still point to tomorrow 6 AM.
According to linked Linear issue SALE-49, this feature is about changing the schedule of automated tasks, so the next-run display should reflect the chosen frequency.</violation>
</file>
<file name="apps/api/src/tasks/dto/swagger.dto.ts">
<violation number="1" location="apps/api/src/tasks/dto/swagger.dto.ts:38">
P2: `integrationScheduleFrequency` is added to the Create and Update Swagger DTOs but is missing from `TaskResponseDto` (both in this file and in `task-responses.dto.ts`). API consumers won't see this field documented in responses, even though the API likely returns it.</violation>
</file>
<file name="apps/app/src/components/schedule-summary.tsx">
<violation number="1" location="apps/app/src/components/schedule-summary.tsx:39">
P2: `toLocaleDateString()` without an explicit locale (and `new Date()` when `lastRunAt` is null) will produce different output on server vs. client, causing a React hydration mismatch. Pass a fixed locale (e.g. `'en-US'`) and consistent date options, or use `suppressHydrationWarning` on the element.</violation>
</file>
<file name="apps/api/src/trigger/shared/is-due-today.ts">
<violation number="1" location="apps/api/src/trigger/shared/is-due-today.ts:5">
P2: `calendarMonthsBetween` ignores the day component, so a "monthly" task that last ran on Jan 31 will fire again on Feb 1 — just 1 day later. The same class of early-trigger affects quarterly and yearly schedules near month boundaries.
Consider comparing elapsed days (like the `weekly` case) or at minimum requiring the day-of-month to have been reached, e.g.:
```ts
function calendarMonthsBetween(earlier: Date, later: Date): number {
const years = later.getUTCFullYear() - earlier.getUTCFullYear();
const months = later.getUTCMonth() - earlier.getUTCMonth();
const total = years * 12 + months;
// Don't count the current month if the day hasn't been reached yet
if (later.getUTCDate() < earlier.getUTCDate()) return total - 1;
return total;
}
```</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
- TaskIntegrationChecks: compute "Next run" from scheduleFrequency + lastRunAt rather than hardcoding daily 6 AM UTC (was misleading for non-daily schedules). - Task response DTOs (swagger.dto.ts + task-responses.dto.ts): expose integrationScheduleFrequency and integrationLastRunAt so API consumers see them in generated documentation. - ScheduleSummary: render locale-agnostic YYYY-MM-DD and anchor "now" to UTC midnight to prevent server/client hydration mismatches. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Thanks @cubic-dev-ai. Addressed 3 of 4 findings in fe921a8:
Not fixing #4 (
Earlier drafts of the plan used the day-of-month adjustment cubic suggests, but it contradicted one of its own test cases ( |
@Marfuen That makes sense—if the design spec explicitly calls for "calendar bucket" logic to match how users view their monthly/quarterly calendars, then the current implementation in I suggested the day-of-month check originally to prevent two runs from occurring too close together (e.g., Jan 31 and Feb 1), but sticking to the "first tick of a new month" behavior is a valid product choice for an MVP, especially since it avoids the complexity of anniversary dates. The test case you highlighted ( The other fixes look solid:
One small observation: in I've noted the "calendar bucket" preference in my memory for this repo so I don't flag it again if you use it in other schedulers. |
The TaskResponseDto now declares integrationScheduleFrequency as a required field; the createTask service method must populate it from the persisted row to satisfy the contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… not enum Replaces EditScheduleDialog with an inline SchedulePicker on the automation Settings tab — fewer clicks, no Dialog focus-trap quirks when interacting with the DS Select. Also forces SelectValue to render the human label (e.g. "Daily") rather than falling back to the raw enum string when re-opened. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Requires human review: Large feature PR (2500+ lines) involving database migrations, core scheduling logic changes, and API updates that require human validation for correctness and safety.
- MetricsSection now takes scheduleFrequency + lastRunAt and re-derives the Schedule and Next Run labels per change. - "Every day at 9:00 AM UTC" → "Every day at 4:00 AM EST" (or whatever the user's locale resolves), and the same TZ is shown on Next Run for consistency. - Both labels are computed client-only to avoid SSR/CSR drift across timezones; placeholder em-dash during SSR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The DS Select uses base-ui internals whose portaled popover fights with the Radix-based Dialog focus trap (closes immediately on click in BrowserAutomationConfigDialog). @trycompai/ui Select is Radix-based like the Dialog, so the two co-operate. Same API shape so callers don't change. Test mock updated accordingly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/automations/[automationId]/overview/components/MetricsSection.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/automations/[automationId]/overview/components/MetricsSection.tsx:40">
P2: When `lastRunAt` is null (never-run automation), `base` falls back to `now` and then the full frequency period is added, pushing the next-run estimate one period too far into the future. The orchestrator would actually pick up a never-run automation on its very next 09:00 UTC tick.
Skip adding the period when `lastRunAt` is null so the function falls into the existing "snap to next 09:00 UTC" branch.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/automations/[automationId]/overview/components/MetricsSection.test.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/automations/[automationId]/overview/components/MetricsSection.test.tsx:56">
P3: Tighten the assertion so it fails on an actual UTC label, not just the phrase `UTC at`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
For null lastRunAt the orchestrator's isDueToday short-circuits to true, so the automation runs on the very next 09:00 UTC tick. The previous math added a full period to "now", over-projecting a never-run weekly automation a full week into the future. Apply the same fix to ScheduleSummary on automation cards. Also tighten the MetricsSection test's UTC-absence assertion to use a word-boundary regex so it can't pass on a label that legitimately contains "UTC". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Requires human review: This PR includes database schema changes (Prisma migration), modifications to core background task orchestration logic, and API shape changes which require human review.
|
🎉 This PR is included in version 3.34.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This is an automated pull request to merge mariano/sale-49-custom-task-schedules into dev.
It was created by the [Auto Pull Request] action.
Summary by cubic
Adds custom schedules for automations and integration checks (daily/weekly/monthly/quarterly/yearly). Orchestrators only run what’s due (via
isDueToday), retry on execution errors, and the app now has inline schedule pickers with next‑run summaries; never‑run items show the next orchestrator tick as their next run. Implements SALE-49.New Features
@db): addscheduleFrequencyandlastRunAtto browser and evidence automations; addintegrationScheduleFrequencyandintegrationLastRunAtto tasks; dropBrowserAutomation.schedule.scheduleFrequencyon browser/evidence automations andintegrationScheduleFrequencyon tasks; addisDueToday; orchestrators filter due items and only advance last‑run on real execution; retry integration checks when any check has execution errors; exposeevaluationStatuson browser runs; includeintegrationScheduleFrequencyandintegrationLastRunAtin task responses and Swagger; ensurecreateTaskreturnsintegrationScheduleFrequency.SchedulePickerandScheduleSummary; inline schedule editing on the Automation Settings tab; enable schedule editing in browser automation create/edit, evidence automation edit, and task integration checks; show schedule summary with a deterministic UTC YYYY‑MM‑DD next run on cards and use the next orchestrator tick when never run; Metrics section derives “Schedule” and “Next run” in the user’s timezone with SSR‑safe placeholders;SchedulePickernow uses@trycompai/uiSelect.Migration
schedulewithscheduleFrequencyon browser automations.scheduleFrequencyorintegrationScheduleFrequency; defaults todailyif unset.Written for commit b8f0081. Summary will update on new commits.