add Manage Automations modal - #46254
Conversation
| @@ -1,29 +1,422 @@ | |||
| import React from "react"; | |||
| /* eslint-disable @typescript-eslint/no-use-before-define */ | |||
There was a problem hiding this comment.
This is to resolve an issue when using the AutomationRowTooltip within ManageAutomationsModal, which is at the bottom of the file.
I prefer having the "main" component (ManageAutomationsModal) at the top and any other inner components at the bottom (treat them as private functions / implementation details).
I don't think it's worth it to move it to a separate file (yet) but it has enough logic that it's worth being its own function/component IMHO.
I have a draft PR proposing this pattern instead of calling render*() functions within the main component: #46213
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## policy-automations-continuous-retries #46254 +/- ##
=========================================================================
- Coverage 66.84% 66.79% -0.05%
=========================================================================
Files 2758 2762 +4
Lines 219781 219956 +175
Branches 11048 11004 -44
=========================================================================
+ Hits 146903 146913 +10
- Misses 59608 59774 +166
+ Partials 13270 13269 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…tries' into 45148-manage-automations-frontend-new-manage
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis PR implements a complete modal UI for managing policy automations, starting with a new Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/pages/policies/ManagePoliciesPage/components/ManageAutomationsModal/types.ts (1)
3-8: ⚡ Quick winConsider exporting
IAutomationRowKeyfor better reusability.While the key type is accessible via
IAutomationRow['key'], directly exportingIAutomationRowKeywould improve developer ergonomics for switch statements, type guards, or pattern matching on automation keys outside this module.📤 Proposed change to export the type
-type IAutomationRowKey = +export type IAutomationRowKey = | "ticket_webhook" | "install_software" | "run_script" | "calendar_event" | "conditional_access";🤖 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 `@frontend/pages/policies/ManagePoliciesPage/components/ManageAutomationsModal/types.ts` around lines 3 - 8, The type IAutomationRowKey is currently internal and should be exported so other modules can reuse it; update the type declaration for IAutomationRowKey to be exported (export type IAutomationRowKey = ...) and ensure any existing imports or usages reference the exported name (e.g., where IAutomationRow['key'] is used, prefer importing IAutomationRowKey where appropriate) to enable clearer switch statements and type guards across the codebase.
🤖 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
`@frontend/pages/policies/ManagePoliciesPage/components/ManageAutomationsModal/hooks/useUpdatePolicyAutomations.ts`:
- Around line 30-37: Guard against missing teamIdForApi for team-scoped updates:
in the hook (useUpdatePolicyAutomations) add an early validation that if
isGlobalPolicy is false and teamIdForApi is undefined, abort the update (call
onError if provided and return) before performing the API call or cache writes;
only include teamIdForApi in the request payload and in any cache key/path
updates when it is defined, and ensure this guard is placed before the existing
logic that references teamIdForApi (the code around the update call and cache
mutations that currently use teamIdForApi at the referenced lines).
---
Nitpick comments:
In
`@frontend/pages/policies/ManagePoliciesPage/components/ManageAutomationsModal/types.ts`:
- Around line 3-8: The type IAutomationRowKey is currently internal and should
be exported so other modules can reuse it; update the type declaration for
IAutomationRowKey to be exported (export type IAutomationRowKey = ...) and
ensure any existing imports or usages reference the exported name (e.g., where
IAutomationRow['key'] is used, prefer importing IAutomationRowKey where
appropriate) to enable clearer switch statements and type guards across the
codebase.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46817ea6-5530-4751-b934-75e1f3d10ef3
📒 Files selected for processing (11)
frontend/interfaces/policy.tsfrontend/pages/policies/ManagePoliciesPage/ManagePoliciesPage.tsxfrontend/pages/policies/ManagePoliciesPage/components/ManageAutomationsModal/ManageAutomationsModal.tsxfrontend/pages/policies/ManagePoliciesPage/components/ManageAutomationsModal/_styles.scssfrontend/pages/policies/ManagePoliciesPage/components/ManageAutomationsModal/helpers.tsfrontend/pages/policies/ManagePoliciesPage/components/ManageAutomationsModal/hooks/index.tsfrontend/pages/policies/ManagePoliciesPage/components/ManageAutomationsModal/hooks/useScripts.tsfrontend/pages/policies/ManagePoliciesPage/components/ManageAutomationsModal/hooks/useSoftwareTitles.tsfrontend/pages/policies/ManagePoliciesPage/components/ManageAutomationsModal/hooks/useUpdatePolicyAutomations.tsfrontend/pages/policies/ManagePoliciesPage/components/ManageAutomationsModal/types.tsfrontend/services/entities/team_policies.ts
| interface IUseUpdatePolicyAutomationsArgs { | ||
| policy: IPolicyStats; | ||
| teamIdForApi: number | undefined; | ||
| isGlobalPolicy: boolean; | ||
| automationsConfig: IConfig | ITeamConfig | undefined; | ||
| onSuccess?: () => void; | ||
| onError?: () => void; | ||
| } |
There was a problem hiding this comment.
Guard team-scoped updates when teamIdForApi is missing.
For non-global policies, teamIdForApi is still optional by type and is used directly at Line 73 and Line 74 (and passed in payload at Line 84). If it’s undefined, updates and cache writes target an invalid key/path.
Suggested hardening
const useUpdatePolicyAutomations = ({
policy,
teamIdForApi,
isGlobalPolicy,
automationsConfig,
onSuccess,
onError,
}: IUseUpdatePolicyAutomationsArgs) => {
const queryClient = useQueryClient();
const { setConfig } = useContext(AppContext);
+
+ if (!isGlobalPolicy && teamIdForApi === undefined) {
+ throw new Error(
+ "teamIdForApi is required for team-scoped policy automation updates."
+ );
+ }
@@
if (isGlobalPolicy) {
const updatedConfig = await configAPI.update(payload);
queryClient.setQueryData(["config"], updatedConfig);
setConfig(updatedConfig);
} else {
- const updatedTeam = await teamsAPI.update(payload, teamIdForApi);
- queryClient.setQueryData(["teams", teamIdForApi], updatedTeam);
+ const updatedTeam = await teamsAPI.update(payload, teamIdForApi!);
+ queryClient.setQueryData(["teams", teamIdForApi!], updatedTeam);
}
};Also applies to: 68-75, 83-85
🤖 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
`@frontend/pages/policies/ManagePoliciesPage/components/ManageAutomationsModal/hooks/useUpdatePolicyAutomations.ts`
around lines 30 - 37, Guard against missing teamIdForApi for team-scoped
updates: in the hook (useUpdatePolicyAutomations) add an early validation that
if isGlobalPolicy is false and teamIdForApi is undefined, abort the update (call
onError if provided and return) before performing the API call or cache writes;
only include teamIdForApi in the request payload and in any cache key/path
updates when it is defined, and ensure this guard is placed before the existing
logic that references teamIdForApi (the code around the update call and cache
mutations that currently use teamIdForApi at the referenced lines).
| const SCRIPTS_PAGE_SIZE = 1000; | ||
|
|
||
| interface IUseScriptsArgs { | ||
| teamId: number; |
| <ManageAutomationsModal | ||
| policy={selectedPolicyForAutomations} | ||
| fleetName={ | ||
| isAllTeamsSelected ? "All fleets" : currentTeamSummary?.name ?? "" |
There was a problem hiding this comment.
If the fleetName values should only constrained to either the fleet name or "All fleets" I don't think this will do the right thing if the user has "Unassigned" selected?
There was a problem hiding this comment.
actually it does set "Unassigned" correctly, however I simplified to:
fleetName={currentTeamSummary?.name ?? ""}There was a problem hiding this comment.
My point was that I don't think we want to display "Unassigned" in this context, AFAIK, when talking about policies either we have global policies (which should render "All fleets") or Fleet policies (which should display the fleet name) - but I see this was fixed in https://github.com/fleetdm/fleet/pull/46271/changes#diff-26bb41183dde00948e910199094c5796925e3219348dd9a9d6a7c8042e87d55bR997
| )} | ||
| </div> | ||
|
|
||
| <div className="modal-cta-wrap"> |
There was a problem hiding this comment.
This is styled using row-reverse, so we actually need the Save button to come before the Cancel button.
| const SOFTWARE_PAGE_SIZE = 1000; | ||
|
|
||
| interface IUseSoftwareTitlesArgs { | ||
| teamId: number; |
| ? { | ||
| software_title_id: installSoftware ? softwareTitleId : null, | ||
| script_id: runScript ? scriptId : null, | ||
| calendar_events_enabled: calendarEvent, |
There was a problem hiding this comment.
Don't you need to also check whether the calendar events are enabled for the team before setting this? isCalendarEnabledForTeam?
| software_title_id: installSoftware ? softwareTitleId : null, | ||
| script_id: runScript ? scriptId : null, | ||
| calendar_events_enabled: calendarEvent, | ||
| conditional_access_enabled: conditionalAccess, |
There was a problem hiding this comment.
Same here, you might also want to check whether cond access is enabled for the team before persisting this isConditionalAccessEnabledForTeam
c083625
into
policy-automations-continuous-retries
…rited policies (#46271) **Related issue:** #45148 and #45145 ## Summary Stacked on top of the Manage automations modal PR (#46254). Two follow-up tweaks: - **Open the modal from the empty ("---") automations cell.** Policies with no automations configured were previously a non-interactive cell. Now, for users who can edit, the empty cell is clickable. - **Handle inherited policies opened from a fleet's list.** An inherited policy has `team_id === null` even when viewed from within a specific fleet. It's now treated as global (only the webhook/ticket row, no continuous-retry option) and the modal is routed to the **global** config for its automations, instead of inferring "global" from the selected-team view. ## Testing - [x] QA'd all new/changed functionality manually https://github.com/user-attachments/assets/9a927373-e1d2-4326-92cc-a97f26d85962 https://github.com/user-attachments/assets/9ff9c393-56c7-4631-9052-477a8736af7a
2 participants
Related issue: Resolves #45148 (just the modal itself, Edit policy and Save policy modal changes will be follow-ups).
Figma: https://www.figma.com/design/QeOcex5LtuDYG9XrGnMFLZ/-42651-Easier-to-manage-policy-automations-with-continuous-retry-option-for-software-scripts?node-id=5373-9851&t=U4xhTwwGzJ9osxYF-0
Testing
Screen.Recording.2026-05-27.at.12.18.57.PM.mov
Summary by CodeRabbit