feat(ui): per-provider scan schedule management gated by capability#11521
Open
alejandrobailo wants to merge 28 commits into
Open
feat(ui): per-provider scan schedule management gated by capability#11521alejandrobailo wants to merge 28 commits into
alejandrobailo wants to merge 28 commits into
Conversation
Schedule domain types and the schedules lib utilities with unit tests.
- Compute the INTERVAL next-run estimate from the backend anchor (next occurrence of scan_hour) instead of now + 48h - Preserve a custom scan_interval_hours when editing instead of silently rewriting it to 48, with a dynamic frequency label - Label the MANUAL_ONLY wizard action "Launch scan" instead of "Save"
|
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:
📝 WalkthroughWalkthroughAdds end-to-end scan scheduling: types and env helper, schedule schema and helpers with tests, server-actions (CRUD) with success-only revalidation, UI editor and fields, save orchestration (optionally launching scans), scans-page enrichment with synthetic pending rows, and many supporting tests and UI updates. ChangesSchedule Management Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
✅ Conflict Markers Resolved All conflict markers have been successfully resolved in this pull request. |
Contributor
|
Please add an entry to the corresponding |
alejandrobailo
added
the
no-changelog
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
🔒 Container Security ScanImage: 📊 Vulnerability Summary
2 package(s) affected
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/components/providers/table/data-table-row-actions.test.tsx (1)
197-223:⚠️ Potential issue | 🟡 MinorStub
NEXT_PUBLIC_IS_CLOUD_ENVfor deterministic “Edit Scan Schedule” gating
DataTableRowActionscomputescanEditSchedulefromcapability ?? getScanScheduleCapability(isCloud()), andisCloud()readsprocess.env.NEXT_PUBLIC_IS_CLOUD_ENV === "true". This test doesn’t stub that env var, so the “Edit Scan Schedule” absence can vary by runner/CI environment—stub it to"false"in the test.🤖 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 `@ui/components/providers/table/data-table-row-actions.test.tsx` around lines 197 - 223, The test fails to stub the cloud env flag so gating for “Edit Scan Schedule” is nondeterministic; before rendering DataTableRowActions in the "renders Add Credentials..." test set NEXT_PUBLIC_IS_CLOUD_ENV to "false" (e.g. via process.env.NEXT_PUBLIC_IS_CLOUD_ENV = "false" or vi.stubEnv) and restore/clear it after the test, ensuring isCloud() (and thus getScanScheduleCapability/canEditSchedule) behaves deterministically for this test.
🤖 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 `@ui/actions/schedules/schedules.ts`:
- Line 20: The three outbound schedule API calls that use fetch(url.toString(),
{ headers }) need an abort timeout: import and use the shared abort helper (or
create an AbortController via the shared helper) and pass its signal into fetch
(e.g., fetch(url.toString(), { headers, signal })), ensure the controller is
aborted on timeout and the timeout cleared on success, and propagate or handle
AbortError appropriately; update all three calls in this file that use fetch to
use the helper so slow/upresponsive upstreams cannot stall execution.
In `@ui/components/shadcn/select/multiselect.tsx`:
- Around line 314-317: The widthClasses string is embedding
var(--radix-popover-trigger-width) into Tailwind arbitrary classes (via the
width variable and widthClasses) which violates the guideline; remove any
var(...) occurrences from the className and move that logic into CSS or inline
styles: create a dedicated CSS rule (or style object) that uses
var(--radix-popover-trigger-width) for the sm: min-width/max-width calculations
(preserving the current wide vs default branches and the sm: min/max behavior),
then update the component to use the clean Tailwind className from widthClasses
(without var(...)) and apply the new CSS class or style to the same element
(refer to widthClasses, width and the element where className is set) so the
visual widths remain identical but no CSS variable is embedded in className.
In `@ui/types/schedules.ts`:
- Around line 44-51: Extract the nested inline types inside
ScheduleRelationships into named interfaces: create a generic RelationshipData
interface (e.g., RelationshipData { type: string; id: string }) or a specific
ProviderRelationshipData for the provider, then replace the inline shape at
provider.data with a reference to that new interface; update
ScheduleRelationships to reference the new interface for provider.data so the
nested structure is declared recursively and consistently (refer to
ScheduleRelationships, provider, data, type, id).
---
Outside diff comments:
In `@ui/components/providers/table/data-table-row-actions.test.tsx`:
- Around line 197-223: The test fails to stub the cloud env flag so gating for
“Edit Scan Schedule” is nondeterministic; before rendering DataTableRowActions
in the "renders Add Credentials..." test set NEXT_PUBLIC_IS_CLOUD_ENV to "false"
(e.g. via process.env.NEXT_PUBLIC_IS_CLOUD_ENV = "false" or vi.stubEnv) and
restore/clear it after the test, ensuring isCloud() (and thus
getScanScheduleCapability/canEditSchedule) behaves deterministically for this
test.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9f513a9a-e6b9-4744-8c29-84a638c2a920
📒 Files selected for processing (18)
ui/actions/schedules/index.tsui/actions/schedules/schedules.test.tsui/actions/schedules/schedules.tsui/components/providers/link-to-scans.tsxui/components/providers/table/data-table-row-actions.test.tsxui/components/providers/table/data-table-row-actions.tsxui/components/providers/wizard/steps/launch-step.test.tsxui/components/providers/wizard/steps/launch-step.tsxui/components/scans/schedule/edit-scan-schedule-modal.tsxui/components/scans/schedule/scan-schedule-fields.tsxui/components/scans/table/scan-jobs-row-actions.test.tsxui/components/scans/table/scan-jobs-row-actions.tsxui/components/shadcn/select/multiselect.tsxui/lib/schedules.test.tsui/lib/schedules.tsui/lib/shared/env.tsui/types/index.tsui/types/schedules.ts
| url.searchParams.set("include", "provider"); | ||
|
|
||
| try { | ||
| const response = await fetch(url.toString(), { headers }); |
There was a problem hiding this comment.
Add request timeouts to outbound schedule API calls.
Line 20, Line 36, and Line 63 call fetch without a timeout. A slow/unresponsive upstream can stall server actions and user flows. Add an abort timeout (shared helper) for all three calls.
Suggested change
+const REQUEST_TIMEOUT_MS = 10_000;
+
+async function fetchWithTimeout(input: string, init: RequestInit = {}) {
+ const controller = new AbortController();
+ const timeout = setTimeout(() => controller.abort(), REQUEST_TIMEOUT_MS);
+ try {
+ return await fetch(input, { ...init, signal: controller.signal });
+ } finally {
+ clearTimeout(timeout);
+ }
+}
+
export const getSchedule = async (providerId: string) => {
@@
- const response = await fetch(url.toString(), { headers });
+ const response = await fetchWithTimeout(url.toString(), { headers });
@@
- const response = await fetch(url.toString(), {
+ const response = await fetchWithTimeout(url.toString(), {
method: "PATCH",
headers,
@@
- const response = await fetch(url.toString(), {
+ const response = await fetchWithTimeout(url.toString(), {
method: "DELETE",
headers,
});Also applies to: 36-36, 63-63
🤖 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 `@ui/actions/schedules/schedules.ts` at line 20, The three outbound schedule
API calls that use fetch(url.toString(), { headers }) need an abort timeout:
import and use the shared abort helper (or create an AbortController via the
shared helper) and pass its signal into fetch (e.g., fetch(url.toString(), {
headers, signal })), ensure the controller is aborted on timeout and the timeout
cleared on success, and propagate or handle AbortError appropriately; update all
three calls in this file that use fetch to use the helper so slow/upresponsive
upstreams cannot stall execution.
Comment on lines
314
to
+317
| const widthClasses = | ||
| width === "wide" | ||
| ? "w-[min(max(var(--radix-popover-trigger-width),24rem),calc(100vw-2rem))] max-w-[32rem]" | ||
| : "w-[min(var(--radix-popover-trigger-width),calc(100vw-2rem))] max-w-[24rem]"; | ||
| ? "w-[calc(100vw-2rem)] sm:w-max sm:min-w-[min(max(var(--radix-popover-trigger-width),24rem),32rem)] sm:max-w-[32rem]" | ||
| : "w-[calc(100vw-2rem)] sm:w-max sm:min-w-[min(var(--radix-popover-trigger-width),22rem)] sm:max-w-[22rem]"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: find className strings containing CSS var(...) in this file.
rg -n 'className=.*var\(--|"\S*var\(--' ui/components/shadcn/select/multiselect.tsxRepository: prowler-cloud/prowler
Length of output: 47
Remove var(--radix-popover-trigger-width) from Tailwind className strings.
ui/components/shadcn/select/multiselect.tsx builds widthClasses with Tailwind arbitrary values containing var(--radix-popover-trigger-width) (lines 316-317) and feeds it into className, which violates the “Never put var(--token) … inside className” guideline. Move the width calculation to style/CSS (using media queries or equivalent) while preserving the existing sm: min/max behavior.
🤖 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 `@ui/components/shadcn/select/multiselect.tsx` around lines 314 - 317, The
widthClasses string is embedding var(--radix-popover-trigger-width) into
Tailwind arbitrary classes (via the width variable and widthClasses) which
violates the guideline; remove any var(...) occurrences from the className and
move that logic into CSS or inline styles: create a dedicated CSS rule (or style
object) that uses var(--radix-popover-trigger-width) for the sm:
min-width/max-width calculations (preserving the current wide vs default
branches and the sm: min/max behavior), then update the component to use the
clean Tailwind className from widthClasses (without var(...)) and apply the new
CSS class or style to the same element (refer to widthClasses, width and the
element where className is set) so the visual widths remain identical but no CSS
variable is embedded in className.
Source: Coding guidelines
Comment on lines
+44
to
+51
| export interface ScheduleRelationships { | ||
| provider: { | ||
| data: { | ||
| type: "providers"; | ||
| id: string; | ||
| }; | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider extracting nested relationship structure into dedicated interfaces.
The ScheduleRelationships interface contains nested objects (provider.data with type and id properties). According to coding guidelines, object properties should use dedicated interfaces recursively rather than inline nested objects. While this structure follows JSON:API conventions, consider refactoring for consistency:
♻️ Suggested refactor for strict guideline compliance
+export interface ProviderRelationshipData {
+ type: "providers";
+ id: string;
+}
+
+export interface ProviderRelationship {
+ data: ProviderRelationshipData;
+}
+
export interface ScheduleRelationships {
- provider: {
- data: {
- type: "providers";
- id: string;
- };
- };
+ provider: ProviderRelationship;
}🤖 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 `@ui/types/schedules.ts` around lines 44 - 51, Extract the nested inline types
inside ScheduleRelationships into named interfaces: create a generic
RelationshipData interface (e.g., RelationshipData { type: string; id: string })
or a specific ProviderRelationshipData for the provider, then replace the inline
shape at provider.data with a reference to that new interface; update
ScheduleRelationships to reference the new interface for provider.data so the
nested structure is declared recursively and consistently (refer to
ScheduleRelationships, provider, data, type, id).
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@ui/components/scans/scans.utils.test.ts`:
- Around line 169-179: The helper makeProvider uses an unsafe "as never" cast
which bypasses TS checks; change it to cast to the real expected type (or a
Partial of it) so tests retain type-safety: update the return cast in
makeProvider to the proper interface/type used by the codebase (or use
Partial<ThatType> if fields are omitted) so TypeScript can validate structure
instead of using "as never".
In `@ui/lib/debug-api-log.ts`:
- Around line 1-33: Remove the temporary API debug logging by deleting or
disabling ui/lib/debug-api-log.ts and removing its usages: replace or remove
imports of logApiRequest, logApiResponse, and logApiError in
ui/actions/scans/scans.ts and ui/actions/schedules/schedules.ts (or switch them
to a no-op shim) so no request/response bodies are logged and the eslint
no-console override/TODO is gone before merge.
In `@ui/lib/schedules.ts`:
- Around line 170-190: describeScheduleCadence presently accepts
ScheduleAttributes that allow nullable scan_hour and relies on callers to only
pass configured schedules; tighten this by narrowing the parameter type to a
non-nullable variant or adding an explicit assertion/check inside
describeScheduleCadence that scan_hour !== null/undefined before formatting.
Update the function signature to accept a ScheduleAttributesWithConfiguredHour
(or similar) or add a guard/assert referencing attributes.scan_hour, and adjust
callers like buildPendingScheduleRows and any uses that call
describeScheduleCadence to satisfy the new type/guard (or perform the null check
and throw/log) so the function cannot produce misleading "@ 12:00am" output for
unconfigured schedules.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 66f825ae-9209-4a0e-bae4-b272f5d55deb
📒 Files selected for processing (14)
ui/actions/scans/scans.tsui/actions/schedules/schedules.tsui/app/(prowler)/scans/page.tsxui/components/providers/wizard/steps/launch-step.tsxui/components/scans/scans.utils.test.tsui/components/scans/scans.utils.tsui/components/scans/schedule/scan-schedule-fields.tsxui/components/scans/table/cells/schedule-cell.tsxui/components/scans/table/scan-jobs-columns.tsxui/components/scans/table/scan-jobs-row-actions.tsxui/lib/debug-api-log.tsui/lib/schedules.tsui/types/scans.tsui/types/schedules.ts
💤 Files with no reviewable changes (1)
- ui/components/providers/wizard/steps/launch-step.tsx
Comment on lines
+169
to
+179
| const makeProvider = (id: string) => | ||
| ({ | ||
| id, | ||
| type: "providers", | ||
| attributes: { | ||
| provider: "aws", | ||
| uid: `uid-${id}`, | ||
| alias: `alias-${id}`, | ||
| }, | ||
| relationships: {}, | ||
| }) as never; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider replacing as never with proper type casting.
The as never cast bypasses TypeScript's type checking entirely. While this works in tests, it's a type-safety anti-pattern that could mask type mismatches.
♻️ Safer alternatives
Option 1: Cast to the actual type:
const makeProvider = (id: string) =>
({
id,
type: "providers",
attributes: {
provider: "aws",
uid: `uid-${id}`,
alias: `alias-${id}`,
},
relationships: {},
- }) as never;
+ }) as ProviderProps;Option 2: Use Partial if you don't need all fields:
const makeProvider = (id: string): Partial<ProviderProps> =>
({
// ...
- }) as never;
+ });🤖 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 `@ui/components/scans/scans.utils.test.ts` around lines 169 - 179, The helper
makeProvider uses an unsafe "as never" cast which bypasses TS checks; change it
to cast to the real expected type (or a Partial of it) so tests retain
type-safety: update the return cast in makeProvider to the proper interface/type
used by the codebase (or use Partial<ThatType> if fields are omitted) so
TypeScript can validate structure instead of using "as never".
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@ui/components/providers/wizard/steps/launch-step.tsx`:
- Around line 139-163: The goToScans ToastAction currently always links to
SCAN_JOBS_TAB.ACTIVE, which is incorrect for non-launched outcomes; update the
logic to compute a targetTab (e.g., const targetTab = result.status ===
SAVE_SCHEDULE_STATUS.SAVED_AND_LAUNCHED ? SCAN_JOBS_TAB.ACTIVE :
SCAN_JOBS_TAB.SCHEDULED) and use that variable in the goToScans Link, and ensure
both the SAVE_SCHEDULE_STATUS.SAVED_SCAN_FAILED branch and the subsequent toast
use this computed goToScans so saved-but-not-launched results route users to the
SCHEDULED tab.
In `@ui/components/scans/launch-scan-modal.tsx`:
- Around line 122-147: In loadSchedule, reset the scheduleForm when the provider
changes and on load error to avoid carrying values from a previously loaded
provider; specifically, when requestedProviderRef.current is set (at start of
load) and when id is falsy, call scheduleForm.reset(...) with a safe
empty/default form (instead of leaving previous values), and when detecting a
failure (the error branch where setScheduleLoad(SCHEDULE_LOAD_STATE.ERROR) is
called) also reset scheduleForm to the empty/default values before returning;
update references in loadSchedule (requestedProviderRef, scheduleForm.reset,
setScheduleLoad, and getSchedule) accordingly so the form is cleared prior to
fetch and on error to prevent stale cross-provider saves.
In `@ui/components/scans/schedule/save-schedule.ts`:
- Around line 1-70: The saveScheduleWithInitialScan orchestration (including
SAVE_SCHEDULE_STATUS, SaveScheduleParams, SaveScheduleResult types and related
helpers) lives in a component folder but is reused; move this function and its
exported constants/types into a new shared module under lib/ (e.g.,
lib/schedules/save-schedule.ts), update callers to import from the new module,
and keep the implementation identical (including useLegacyDaily branch,
scheduleDaily/updateSchedule calls, and scanOnDemand flow); ensure exports
remain named the same so consumers (components under
ui/components/scans/schedule/) only need to change import paths and run tests to
verify nothing else breaks.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f2bdc8db-26db-4b9c-867f-e21afdfe1b6a
📒 Files selected for processing (10)
ui/actions/scans/scans.tsui/actions/schedules/schedules.tsui/components/providers/wizard/steps/launch-step.tsxui/components/scans/launch-scan-modal.test.tsxui/components/scans/launch-scan-modal.tsxui/components/scans/scans-page-shell.tsxui/components/scans/schedule/save-schedule.test.tsui/components/scans/schedule/save-schedule.tsui/components/scans/schedule/scan-schedule-fields.tsxui/components/scans/table/cells/scan-info-cell.tsx
💤 Files with no reviewable changes (2)
- ui/actions/scans/scans.ts
- ui/actions/schedules/schedules.ts
Comment on lines
+139
to
+163
| const goToScans = ( | ||
| <ToastAction altText="Go to scans" asChild> | ||
| <Link href={`/scans?tab=${SCAN_JOBS_TAB.ACTIVE}`}>Go to scans</Link> | ||
| </ToastAction> | ||
| ); | ||
|
|
||
| if (result.status === SAVE_SCHEDULE_STATUS.SAVED_SCAN_FAILED) { | ||
| toast({ | ||
| title: "Scan schedule saved", | ||
| description: `The schedule was saved, but the initial scan could not be launched: ${result.message}`, | ||
| action: goToScans, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const launched = result.status === SAVE_SCHEDULE_STATUS.SAVED_AND_LAUNCHED; | ||
| toast({ | ||
| title: launched | ||
| ? "Scan schedule saved and initial scan launched" | ||
| : "Scan schedule saved", | ||
| description: launched | ||
| ? "The schedule was saved and the initial scan was launched." | ||
| : "The scan schedule was saved successfully.", | ||
| action: goToScans, | ||
| }); |
There was a problem hiding this comment.
Route non-launched schedule outcomes to the Scheduled tab.
The shared goToScans action always links to ACTIVE, but SAVED and SAVED_SCAN_FAILED can complete without creating an active scan. This sends users to the wrong destination.
Suggested fix
- const goToScans = (
+ const launched = result.status === SAVE_SCHEDULE_STATUS.SAVED_AND_LAUNCHED;
+ const goToScans = (
<ToastAction altText="Go to scans" asChild>
- <Link href={`/scans?tab=${SCAN_JOBS_TAB.ACTIVE}`}>Go to scans</Link>
+ <Link
+ href={`/scans?tab=${
+ launched ? SCAN_JOBS_TAB.ACTIVE : SCAN_JOBS_TAB.SCHEDULED
+ }`}
+ >
+ Go to scans
+ </Link>
</ToastAction>
);
@@
- const launched = result.status === SAVE_SCHEDULE_STATUS.SAVED_AND_LAUNCHED;
toast({📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const goToScans = ( | |
| <ToastAction altText="Go to scans" asChild> | |
| <Link href={`/scans?tab=${SCAN_JOBS_TAB.ACTIVE}`}>Go to scans</Link> | |
| </ToastAction> | |
| ); | |
| if (result.status === SAVE_SCHEDULE_STATUS.SAVED_SCAN_FAILED) { | |
| toast({ | |
| title: "Scan schedule saved", | |
| description: `The schedule was saved, but the initial scan could not be launched: ${result.message}`, | |
| action: goToScans, | |
| }); | |
| return; | |
| } | |
| const launched = result.status === SAVE_SCHEDULE_STATUS.SAVED_AND_LAUNCHED; | |
| toast({ | |
| title: launched | |
| ? "Scan schedule saved and initial scan launched" | |
| : "Scan schedule saved", | |
| description: launched | |
| ? "The schedule was saved and the initial scan was launched." | |
| : "The scan schedule was saved successfully.", | |
| action: goToScans, | |
| }); | |
| const launched = result.status === SAVE_SCHEDULE_STATUS.SAVED_AND_LAUNCHED; | |
| const goToScans = ( | |
| <ToastAction altText="Go to scans" asChild> | |
| <Link | |
| href={`/scans?tab=${ | |
| launched ? SCAN_JOBS_TAB.ACTIVE : SCAN_JOBS_TAB.SCHEDULED | |
| }`} | |
| > | |
| Go to scans | |
| </Link> | |
| </ToastAction> | |
| ); | |
| if (result.status === SAVE_SCHEDULE_STATUS.SAVED_SCAN_FAILED) { | |
| toast({ | |
| title: "Scan schedule saved", | |
| description: `The schedule was saved, but the initial scan could not be launched: ${result.message}`, | |
| action: goToScans, | |
| }); | |
| return; | |
| } | |
| toast({ | |
| title: launched | |
| ? "Scan schedule saved and initial scan launched" | |
| : "Scan schedule saved", | |
| description: launched | |
| ? "The schedule was saved and the initial scan was launched." | |
| : "The scan schedule was saved successfully.", | |
| action: goToScans, | |
| }); |
🤖 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 `@ui/components/providers/wizard/steps/launch-step.tsx` around lines 139 - 163,
The goToScans ToastAction currently always links to SCAN_JOBS_TAB.ACTIVE, which
is incorrect for non-launched outcomes; update the logic to compute a targetTab
(e.g., const targetTab = result.status ===
SAVE_SCHEDULE_STATUS.SAVED_AND_LAUNCHED ? SCAN_JOBS_TAB.ACTIVE :
SCAN_JOBS_TAB.SCHEDULED) and use that variable in the goToScans Link, and ensure
both the SAVE_SCHEDULE_STATUS.SAVED_SCAN_FAILED branch and the subsequent toast
use this computed goToScans so saved-but-not-launched results route users to the
SCHEDULED tab.
Comment on lines
+122
to
+147
| const loadSchedule = async (id: string) => { | ||
| requestedProviderRef.current = id; | ||
| if (!id) { | ||
| setScheduleLoad(SCHEDULE_LOAD_STATE.IDLE); | ||
| return; | ||
| } | ||
|
|
||
| setScheduleLoad(SCHEDULE_LOAD_STATE.LOADING); | ||
| const response = (await getSchedule(id)) as | ||
| | ScheduleApiResponse | ||
| | { error?: string }; | ||
|
|
||
| if (requestedProviderRef.current !== id) return; | ||
|
|
||
| if (!response || ("error" in response && response.error)) { | ||
| setScheduleLoad(SCHEDULE_LOAD_STATE.ERROR); | ||
| return; | ||
| } | ||
|
|
||
| scheduleForm.reset( | ||
| getScheduleFormValues( | ||
| "data" in response ? response.data?.attributes : null, | ||
| ), | ||
| ); | ||
| setScheduleLoad(SCHEDULE_LOAD_STATE.LOADED); | ||
| }; |
There was a problem hiding this comment.
Reset schedule form state on provider switch/load failure to prevent stale cross-provider saves.
When getSchedule fails after a prior provider load, scheduleForm keeps the previous provider’s values while remaining editable, so saving can apply stale cadence data to the current provider. Reset the schedule form when provider changes (before fetch) and on error.
Proposed fix
const loadSchedule = async (id: string) => {
requestedProviderRef.current = id;
if (!id) {
+ scheduleForm.reset(getScheduleFormDefaults());
setScheduleLoad(SCHEDULE_LOAD_STATE.IDLE);
return;
}
+ // Clear stale values from previously selected provider while loading.
+ scheduleForm.reset(getScheduleFormDefaults());
setScheduleLoad(SCHEDULE_LOAD_STATE.LOADING);
const response = (await getSchedule(id)) as
| ScheduleApiResponse
| { error?: string };
if (requestedProviderRef.current !== id) return;
if (!response || ("error" in response && response.error)) {
+ scheduleForm.reset(getScheduleFormDefaults());
setScheduleLoad(SCHEDULE_LOAD_STATE.ERROR);
return;
}
scheduleForm.reset(🤖 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 `@ui/components/scans/launch-scan-modal.tsx` around lines 122 - 147, In
loadSchedule, reset the scheduleForm when the provider changes and on load error
to avoid carrying values from a previously loaded provider; specifically, when
requestedProviderRef.current is set (at start of load) and when id is falsy,
call scheduleForm.reset(...) with a safe empty/default form (instead of leaving
previous values), and when detecting a failure (the error branch where
setScheduleLoad(SCHEDULE_LOAD_STATE.ERROR) is called) also reset scheduleForm to
the empty/default values before returning; update references in loadSchedule
(requestedProviderRef, scheduleForm.reset, setScheduleLoad, and getSchedule)
accordingly so the form is cleared prior to fetch and on error to prevent stale
cross-provider saves.
Comment on lines
+1
to
+70
| import { scanOnDemand, scheduleDaily } from "@/actions/scans"; | ||
| import { updateSchedule } from "@/actions/schedules"; | ||
| import { buildScheduleUpdatePayload } from "@/lib/schedules"; | ||
| import type { ScheduleFormValues } from "@/types/schedules"; | ||
|
|
||
| export const SAVE_SCHEDULE_STATUS = { | ||
| ERROR: "error", | ||
| SAVED: "saved", | ||
| SAVED_AND_LAUNCHED: "saved_and_launched", | ||
| SAVED_SCAN_FAILED: "saved_scan_failed", | ||
| } as const; | ||
|
|
||
| export type SaveScheduleStatus = | ||
| (typeof SAVE_SCHEDULE_STATUS)[keyof typeof SAVE_SCHEDULE_STATUS]; | ||
|
|
||
| export interface SaveScheduleParams { | ||
| providerId: string; | ||
| values: ScheduleFormValues; | ||
| /** Save through the legacy `/schedules/daily` endpoint (OSS / non-Cloud). */ | ||
| useLegacyDaily?: boolean; | ||
| } | ||
|
|
||
| export interface SaveScheduleResult { | ||
| status: SaveScheduleStatus; | ||
| message?: string; | ||
| } | ||
|
|
||
| /** Saves a provider's scan schedule and optionally launches the initial scan. */ | ||
| export async function saveScheduleWithInitialScan({ | ||
| providerId, | ||
| values, | ||
| useLegacyDaily = false, | ||
| }: SaveScheduleParams): Promise<SaveScheduleResult> { | ||
| let scheduleResult: { error?: unknown } | null; | ||
|
|
||
| if (useLegacyDaily) { | ||
| const formData = new FormData(); | ||
| formData.set("providerId", providerId); | ||
| scheduleResult = await scheduleDaily(formData); | ||
| } else { | ||
| scheduleResult = await updateSchedule( | ||
| providerId, | ||
| buildScheduleUpdatePayload(values), | ||
| ); | ||
| } | ||
|
|
||
| if (scheduleResult?.error) { | ||
| return { | ||
| status: SAVE_SCHEDULE_STATUS.ERROR, | ||
| message: String(scheduleResult.error), | ||
| }; | ||
| } | ||
|
|
||
| if (!values.launchInitialScan) { | ||
| return { status: SAVE_SCHEDULE_STATUS.SAVED }; | ||
| } | ||
|
|
||
| const formData = new FormData(); | ||
| formData.set("providerId", providerId); | ||
| const scanResult = await scanOnDemand(formData); | ||
|
|
||
| if (scanResult?.error) { | ||
| return { | ||
| status: SAVE_SCHEDULE_STATUS.SAVED_SCAN_FAILED, | ||
| message: String(scanResult.error), | ||
| }; | ||
| } | ||
|
|
||
| return { status: SAVE_SCHEDULE_STATUS.SAVED_AND_LAUNCHED }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Move this orchestration to a shared lib/ module.
saveScheduleWithInitialScan is already reused across multiple features, so keeping it inside ui/components/scans/schedule/ blurs component-vs-shared boundaries and conflicts with the repository placement rule.
As per coding guidelines, “Shared utilities (used 2+ places) go in lib/ directory; local utilities (1 place) go in {feature}/utils/.”
🤖 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 `@ui/components/scans/schedule/save-schedule.ts` around lines 1 - 70, The
saveScheduleWithInitialScan orchestration (including SAVE_SCHEDULE_STATUS,
SaveScheduleParams, SaveScheduleResult types and related helpers) lives in a
component folder but is reused; move this function and its exported
constants/types into a new shared module under lib/ (e.g.,
lib/schedules/save-schedule.ts), update callers to import from the new module,
and keep the implementation identical (including useLegacyDaily branch,
scheduleDaily/updateSchedule calls, and scanOnDemand flow); ensure exports
remain named the same so consumers (components under
ui/components/scans/schedule/) only need to change import paths and run tests to
verify nothing else breaks.
Source: Coding guidelines
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/components/scans/schedule/edit-scan-schedule-modal.tsx (1)
95-112:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd try-catch to prevent stuck UI if removeSchedule throws.
If
removeSchedulethrows an exception (e.g., network error) rather than returning{error},isRemovingwill never be reset and the UI will remain disabled indefinitely. Wrap the call in try-catch to ensure cleanup always runs.🛡️ Proposed fix
const handleRemove = async () => { setIsRemoving(true); - const result = await removeSchedule(provider.providerId); - setIsRemoving(false); setIsConfirmRemoveOpen(false); - if (result?.error) { - form.setError("root", { message: String(result.error) }); - return; - } + try { + const result = await removeSchedule(provider.providerId); + + if (result?.error) { + form.setError("root", { message: String(result.error) }); + return; + } - toast({ - title: "Scan schedule removed", - description: "The scan schedule was removed successfully.", - }); - onClose(); - router.refresh(); + toast({ + title: "Scan schedule removed", + description: "The scan schedule was removed successfully.", + }); + onClose(); + router.refresh(); + } catch (error) { + form.setError("root", { + message: error instanceof Error ? error.message : "Failed to remove schedule" + }); + } finally { + setIsRemoving(false); + } };🤖 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 `@ui/components/scans/schedule/edit-scan-schedule-modal.tsx` around lines 95 - 112, The handleRemove function should guard against exceptions from removeSchedule by wrapping the await removeSchedule(provider.providerId) call in a try-catch-finally: call removeSchedule inside try, handle thrown errors in catch by calling form.setError("root", { message: String(err) }) (or show a toast) and ensure cleanup (setIsRemoving(false) and setIsConfirmRemoveOpen(false)) runs in finally; keep existing success flow (toast, onClose, router.refresh) only when no error occurs. Ensure you reference handleRemove, removeSchedule, setIsRemoving, setIsConfirmRemoveOpen, form.setError, toast, onClose, and router.refresh when updating the function.
♻️ Duplicate comments (1)
ui/components/scans/scans.utils.test.ts (1)
169-179: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winThe
as nevercast bypasses type safety (duplicate concern).This is the same unsafe cast pattern flagged in the previous review. The
as nevercast completely bypasses TypeScript's type checking, which could mask type mismatches if theProviderPropsstructure changes.🤖 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 `@ui/components/scans/scans.utils.test.ts` around lines 169 - 179, The makeProvider helper currently uses an unsafe cast `as never`; replace it with a properly typed return (e.g. use the actual ProviderProps type or Partial<ProviderProps>) so TypeScript can validate shape changes — update the makeProvider signature to return ProviderProps or Partial<ProviderProps> (and import/define ProviderProps if needed), and adjust the object fields to match ProviderProps (including relationships) instead of casting to never.
🤖 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.
Outside diff comments:
In `@ui/components/scans/schedule/edit-scan-schedule-modal.tsx`:
- Around line 95-112: The handleRemove function should guard against exceptions
from removeSchedule by wrapping the await removeSchedule(provider.providerId)
call in a try-catch-finally: call removeSchedule inside try, handle thrown
errors in catch by calling form.setError("root", { message: String(err) }) (or
show a toast) and ensure cleanup (setIsRemoving(false) and
setIsConfirmRemoveOpen(false)) runs in finally; keep existing success flow
(toast, onClose, router.refresh) only when no error occurs. Ensure you reference
handleRemove, removeSchedule, setIsRemoving, setIsConfirmRemoveOpen,
form.setError, toast, onClose, and router.refresh when updating the function.
---
Duplicate comments:
In `@ui/components/scans/scans.utils.test.ts`:
- Around line 169-179: The makeProvider helper currently uses an unsafe cast `as
never`; replace it with a properly typed return (e.g. use the actual
ProviderProps type or Partial<ProviderProps>) so TypeScript can validate shape
changes — update the makeProvider signature to return ProviderProps or
Partial<ProviderProps> (and import/define ProviderProps if needed), and adjust
the object fields to match ProviderProps (including relationships) instead of
casting to never.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3dbb7635-9510-4f6e-8cde-a6afb059ddff
📒 Files selected for processing (6)
ui/components/scans/scans.utils.test.tsui/components/scans/scans.utils.tsui/components/scans/schedule/edit-scan-schedule-modal.test.tsxui/components/scans/schedule/edit-scan-schedule-modal.tsxui/components/scans/schedule/scan-schedule-fields.tsxui/components/scans/table/cells/scan-info-cell.tsx
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
2 participants
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.
Context
Providers could only be put on a fixed daily scheduled scan from the add-provider wizard. This PR adds full scan-schedule management to the UI (daily, weekly, monthly, or fixed interval), backed by the per-provider
/schedulesAPI and gated by a capability seam so Prowler OSS keeps its legacy daily behavior.Description
schedulesserver actions (GET/PATCH/DELETE /schedules/{providerId}) with shared types (ui/types/schedules.ts) and pure helpers (ui/lib/schedules.ts) for form mapping, payload building, and next-run estimation.ScanScheduleFieldsandEditScanScheduleModalcomponents, reused by the provider wizardLaunchStepand by new "Edit Scan Schedule" row actions in the Providers and Scan Jobs tables.SCAN_SCHEDULE_CAPABILITY:DAILY_LEGACY(OSS) keeps the legacyPOST /schedules/dailyflow with advanced cadences locked behind a Cloud upsell badge;ADVANCEDenables full scheduling;MANUAL_ONLY(prop-driven only) allows just an on-demand scan, honoringisScanLimitReached.scan_interval_hoursinstead of rewriting it to 48.isCloud()env helper, and scan-jobs navigation now pre-selects the provider through theprovider_uid__infilter key with URL-safe encoding.Steps to review
pnpm vitest run lib/schedules.test.ts components/providers/wizard/steps/launch-step.test.tsx components/providers/table/data-table-row-actions.test.tsx components/scans/table/scan-jobs-row-actions.test.tsx(59 tests) pluspnpm run typecheckandpnpm run lint.NEXT_PUBLIC_IS_CLOUD_ENVunset): in the add-provider wizard launch step, the cadence/time selects are locked to Daily with the Cloud badge, saving calls the legacyPOST /schedules/daily, and the "Edit Scan Schedule" row action is hidden in both the Providers and Scan Jobs tables.NEXT_PUBLIC_IS_CLOUD_ENV=true): open "Edit Scan Schedule" from either table and verify thePATCH /schedules/{providerId}payload per cadence — unused shape fields sent asnull, weekday0=Sunday, day-of-month1-28, browser IANA timezone — and that "Remove Scan Schedule" issues theDELETEand the modal then opens with defaults.ui/lib/schedules.tsandui/types/schedules.tsfor the capability seam (capabilityprop +getScanScheduleCapability) that the cloud overlay will inject into.Checklist
Community Checklist
SDK/CLI
UI
API
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Summary by CodeRabbit
New Features
Improvements