Add include_all label scope UI to policies and reports (#41565)#44503
Add include_all label scope UI to policies and reports (#41565)#44503juan-fdz-hawa wants to merge 2 commits intomainfrom
Conversation
![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.
juan-fdz-hawa
had a problem deploying
to
Docker Hub
GitHub Actions
Failure
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #44503 +/- ##
=======================================
Coverage 66.79% 66.79%
=======================================
Files 2637 2637
Lines 212132 212183 +51
Branches 9555 9565 +10
=======================================
+ Hits 141688 141726 +38
- Misses 57578 57591 +13
Partials 12866 12866
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:
|
|
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:
WalkthroughAdds a premium-gated “Include all” label-scope across policy and query UIs and persists it end-to-end. Introduces Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@changes/41565-policy-report-new-scopes-frontend`:
- Around line 1-2: Update the two release-note bullets to correct grammar and
clarity: change the first bullet from "Added a \"Custom\" target dropdown
reports, with a \"Include all\" option for premium users." to "Added a
\"Custom\" target dropdown to reports, with an \"Include all\" option for
premium users." and change the second bullet so it uses "an" correctly (e.g.,
"Added \"Include all\" to the \"Custom\" target dropdown on Policies for premium
users only.")—locate the exact strings "Added a \"Custom\" target dropdown
reports, with a \"Include all\" option for premium users." and "Added \"Include
All\" to \"Custom\" target dropdown on Policies for premium users only." and
replace them with the improved wording.
In `@frontend/pages/policies/details/PolicyDetailsPage/PolicyDetailsPage.tsx`:
- Around line 135-137: The policy details page is storing labels_include_all
(setLastEditedQueryLabelsIncludeAll) but the UI still only renders
labels_include_any / labels_exclude_any; update the PolicyDetailsPage render
logic to read and display the labels_include_all state (e.g.,
lastEditedQuery.labels_include_all or the state variable
setLastEditedQueryLabelsIncludeAll) alongside the other label scopes so policies
using include-all show correct targeting info—add a UI entry for the include-all
scope in the labels section where labels_include_any / labels_exclude_any are
rendered.
🪄 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: 52a841a2-dcbf-40cb-a47d-50ec28827868
📒 Files selected for processing (15)
changes/41565-policy-report-new-scopes-frontendfrontend/context/policy.tsxfrontend/interfaces/policy.tsfrontend/interfaces/schedulable_query.tsfrontend/pages/policies/details/PolicyDetailsPage/PolicyDetailsPage.tsxfrontend/pages/policies/edit/EditPolicyPage.tsxfrontend/pages/policies/edit/components/PolicyForm/PolicyForm.tests.tsxfrontend/pages/policies/edit/components/PolicyForm/PolicyForm.tsxfrontend/pages/policies/edit/components/SaveNewPolicyModal/SaveNewPolicyModal.tsxfrontend/pages/policies/helpers.tsxfrontend/pages/policies/live/LivePolicyPage/LivePolicyPage.tsxfrontend/pages/queries/edit/components/EditQueryForm/EditQueryForm.tsxfrontend/pages/queries/edit/components/SaveNewQueryModal/SaveNewQueryModal.tsxfrontend/pages/queries/helpers.tsxfrontend/services/entities/team_policies.ts
juan-fdz-hawa
had a problem deploying
to
Docker Hub
GitHub Actions
Failure
juan-fdz-hawa
had a problem deploying
to
Docker Hub
GitHub Actions
Failure
juan-fdz-hawa
had a problem deploying
to
Docker Hub
GitHub Actions
Failure
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)
frontend/pages/policies/edit/components/SaveNewPolicyModal/SaveNewPolicyModal.tsx (1)
283-296:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDisable the modal target selector in GitOps mode.
disableOptionsonly tracks the autofill loading state here, so this new label-scope control stays interactive in the save modal while GitOps mode is on. The edit form already disables the same selector viagitOpsModeEnabled; please thread that flag into this modal and include it in the disable condition too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/policies/edit/components/SaveNewPolicyModal/SaveNewPolicyModal.tsx` around lines 283 - 296, The TargetLabelSelector in SaveNewPolicyModal remains interactive in GitOps mode because disableOptions only reflects autofill loading; thread the existing gitOpsModeEnabled flag into this modal (SaveNewPolicyModal) and update the TargetLabelSelector disable condition to include it (i.e. pass disableOptions={disableOptions || gitOpsModeEnabled} or equivalent). Locate the TargetLabelSelector usage in SaveNewPolicyModal and add the gitOpsModeEnabled prop usage so the selector is disabled when gitOpsModeEnabled is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@frontend/pages/policies/edit/components/SaveNewPolicyModal/SaveNewPolicyModal.tsx`:
- Around line 283-296: The TargetLabelSelector in SaveNewPolicyModal remains
interactive in GitOps mode because disableOptions only reflects autofill
loading; thread the existing gitOpsModeEnabled flag into this modal
(SaveNewPolicyModal) and update the TargetLabelSelector disable condition to
include it (i.e. pass disableOptions={disableOptions || gitOpsModeEnabled} or
equivalent). Locate the TargetLabelSelector usage in SaveNewPolicyModal and add
the gitOpsModeEnabled prop usage so the selector is disabled when
gitOpsModeEnabled is true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ade471fd-fc04-493c-95bf-1d87207a3dbf
📒 Files selected for processing (2)
frontend/pages/policies/edit/components/PolicyForm/PolicyForm.tsxfrontend/pages/policies/edit/components/SaveNewPolicyModal/SaveNewPolicyModal.tsx
juan-fdz-hawa
force-pushed
the
41565-policyreport-labels-frontend
branch
from
a921a80 to
ef4fde8
Compare
juan-fdz-hawa
had a problem deploying
to
Docker Hub
GitHub Actions
Failure
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)
frontend/pages/queries/edit/components/SaveNewQueryModal/SaveNewQueryModal.tsx (1)
184-215:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOmit the inactive scope from the save payload.
labels_include_anyandlabels_include_allare both emitted here, with the inactive one set to[]. That still serializes both scopes and can make the backend infer intent from empty arrays instead of a single active field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/queries/edit/components/SaveNewQueryModal/SaveNewQueryModal.tsx` around lines 184 - 215, The payload currently always includes both labels_include_any and labels_include_all (one as an empty array) from SaveNewQueryModal.tsx, which can confuse the backend; update labelsForScope (used to compute labelsIncludeAny/labelsIncludeAll) to return undefined for the inactive scope (not []). Then change the saveQuery payload construction in the SaveNewQueryModal component to only include labels_include_any or labels_include_all when their computed value is defined (omit the inactive key entirely) so the request contains only the active label scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/pages/policies/edit/components/PolicyForm/PolicyForm.tsx`:
- Around line 296-333: The effect in PolicyForm that sets
selectedTargetType/selectedCustomTarget/selectedLabels leaves
selectedCustomTarget unchanged when no label scope is active, causing the wrong
custom scope to persist; update the useEffect so that when no customTarget is
determined you explicitly clear selectedCustomTarget (e.g.,
setSelectedCustomTarget(undefined) or an empty string) and set selectedLabels to
an empty object, keeping the existing logic that sets activeLabels when a
customTarget exists; apply the same fix to the other matching effect that also
assigns selectedCustomTarget/selectedLabels.
In `@frontend/pages/queries/edit/components/EditQueryForm/EditQueryForm.tsx`:
- Around line 240-253: When building the payload passed to updateQueryData,
don't always serialize both labels_include_any and labels_include_all; instead
include only the active scope key. Use getLabelsForScope to compute the labels
for the active scope and conditionally attach either labels_include_any or
labels_include_all (based on the state that tracks the active scope, e.g.,
selected label-scope variable) to the object you pass into updateQueryData,
omitting the inactive key entirely.
- Around line 735-747: EditQueryForm renders TargetLabelSelector without passing
the disableOptions prop so scope controls remain editable in GitOps mode; update
the TargetLabelSelector invocation inside EditQueryForm to include
disableOptions={gitOpsModeEnabled} (same guard used by PolicyForm) so when
gitOpsModeEnabled is true the selector options are disabled and scope control is
locked down. Ensure you reference the existing selectedTargetType,
selectedCustomTarget, and other props remain unchanged and only add the
disableOptions prop to TargetLabelSelector.
---
Outside diff comments:
In
`@frontend/pages/queries/edit/components/SaveNewQueryModal/SaveNewQueryModal.tsx`:
- Around line 184-215: The payload currently always includes both
labels_include_any and labels_include_all (one as an empty array) from
SaveNewQueryModal.tsx, which can confuse the backend; update labelsForScope
(used to compute labelsIncludeAny/labelsIncludeAll) to return undefined for the
inactive scope (not []). Then change the saveQuery payload construction in the
SaveNewQueryModal component to only include labels_include_any or
labels_include_all when their computed value is defined (omit the inactive key
entirely) so the request contains only the active label scope.
🪄 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: c537b205-c112-4a26-8a7f-51063cf4c886
📒 Files selected for processing (15)
changes/41565-policy-report-new-scopes-frontendfrontend/context/policy.tsxfrontend/interfaces/policy.tsfrontend/interfaces/schedulable_query.tsfrontend/pages/policies/details/PolicyDetailsPage/PolicyDetailsPage.tsxfrontend/pages/policies/edit/EditPolicyPage.tsxfrontend/pages/policies/edit/components/PolicyForm/PolicyForm.tests.tsxfrontend/pages/policies/edit/components/PolicyForm/PolicyForm.tsxfrontend/pages/policies/edit/components/SaveNewPolicyModal/SaveNewPolicyModal.tsxfrontend/pages/policies/helpers.tsxfrontend/pages/policies/live/LivePolicyPage/LivePolicyPage.tsxfrontend/pages/queries/edit/components/EditQueryForm/EditQueryForm.tsxfrontend/pages/queries/edit/components/SaveNewQueryModal/SaveNewQueryModal.tsxfrontend/pages/queries/helpers.tsxfrontend/services/entities/team_policies.ts
✅ Files skipped from review due to trivial changes (3)
- changes/41565-policy-report-new-scopes-frontend
- frontend/interfaces/policy.ts
- frontend/pages/queries/helpers.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/pages/policies/edit/components/SaveNewPolicyModal/SaveNewPolicyModal.tsx
- frontend/pages/policies/edit/components/PolicyForm/PolicyForm.tests.tsx
juan-fdz-hawa
force-pushed
the
41565-policyreport-labels-frontend
branch
from
ef4fde8 to
695cf83
Compare
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/pages/policies/edit/EditPolicyPage.tsx`:
- Around line 174-176: The component hydrates labels_include_all into state via
setLastEditedQueryLabelsIncludeAll(returnedQuery.labels_include_all || []), but
the teardown only resets critical/platform state so the include-all draft can
leak into other editor flows; update the component cleanup (the unmount/effect
cleanup in EditPolicyPage) to also reset the include-all draft by calling
setLastEditedQueryLabelsIncludeAll([]) (or the appropriate PolicyContext reset
helper) so labels_include_all is cleared when the editor unmounts.
🪄 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: 67f372cf-7261-4a3a-9cd6-62543638910f
📒 Files selected for processing (15)
changes/41565-policy-report-new-scopes-frontendfrontend/context/policy.tsxfrontend/interfaces/policy.tsfrontend/interfaces/schedulable_query.tsfrontend/pages/policies/details/PolicyDetailsPage/PolicyDetailsPage.tsxfrontend/pages/policies/edit/EditPolicyPage.tsxfrontend/pages/policies/edit/components/PolicyForm/PolicyForm.tests.tsxfrontend/pages/policies/edit/components/PolicyForm/PolicyForm.tsxfrontend/pages/policies/edit/components/SaveNewPolicyModal/SaveNewPolicyModal.tsxfrontend/pages/policies/helpers.tsxfrontend/pages/policies/live/LivePolicyPage/LivePolicyPage.tsxfrontend/pages/queries/edit/components/EditQueryForm/EditQueryForm.tsxfrontend/pages/queries/edit/components/SaveNewQueryModal/SaveNewQueryModal.tsxfrontend/pages/queries/helpers.tsxfrontend/services/entities/team_policies.ts
✅ Files skipped from review due to trivial changes (4)
- frontend/pages/policies/live/LivePolicyPage/LivePolicyPage.tsx
- changes/41565-policy-report-new-scopes-frontend
- frontend/interfaces/schedulable_query.ts
- frontend/interfaces/policy.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- frontend/services/entities/team_policies.ts
- frontend/pages/policies/details/PolicyDetailsPage/PolicyDetailsPage.tsx
- frontend/pages/queries/helpers.tsx
- frontend/pages/queries/edit/components/SaveNewQueryModal/SaveNewQueryModal.tsx
- frontend/pages/policies/edit/components/SaveNewPolicyModal/SaveNewPolicyModal.tsx
- frontend/pages/policies/edit/components/PolicyForm/PolicyForm.tests.tsx
- frontend/pages/queries/edit/components/EditQueryForm/EditQueryForm.tsx
- frontend/pages/policies/edit/components/PolicyForm/PolicyForm.tsx
Surfaces the new include_all label scope on the policy and report (query) edit forms via a "Custom" target dropdown.
juan-fdz-hawa
force-pushed
the
41565-policyreport-labels-frontend
branch
from
695cf83 to
caeb314
Compare
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
nulmete
left a comment
nulmete
left a comment
There was a problem hiding this comment.
Haven't tested locally but code LGTM 👍
I have a few non-blocking comments.
| selectedCustomTarget: string, | ||
| selectedLabels: Record<string, boolean>, | ||
| scope: string |
There was a problem hiding this comment.
Worth defining a common type for selectedCustomTarget and scope? I see that these can be one of these: "labelsIncludeAny", "labelsIncludeAll" or "labelsExcludeAny".
(Maybe we could reuse it in other places, not sure -- I see that we also declare these values when setting up the dropdown options in one of the helpers.tsx file above. If IDropdownOption accepted a generic we could get some stronger typing there instead of plain strings.)
| const [selectedCustomTarget, setSelectedCustomTarget] = useState( | ||
| "labelsIncludeAny" | ||
| ); |
There was a problem hiding this comment.
nit: related to my comment above, we could type this as useState here, where T would be the type I suggested introducing (omitting excludeAny, though).
| .filter(([, selected]) => selected) | ||
| .map(([labelName]) => labelName) | ||
| : []; | ||
| const labelsForScope = (scope: string): string[] | undefined => { |
There was a problem hiding this comment.
nit: do we need the string[] | undefined explicit return type? I think the type should be inferred by TS.
| const labelsForScope = (scope: string): string[] | undefined => { | |
| const labelsForScope = (scope: string) => { |
| @@ -0,0 +1,39 @@ | |||
| import React from "react"; | |||
There was a problem hiding this comment.
nit: is there any opportunity for reusability between this file and the one in the policies directory?
AFAICS, the only things that change are:
- The entity in the helpText ("Report" vs "Policy")
- The exported function (though in policies we export "getCustomTargetOptions" and here "getQueryCustomTargetOptions"). Should we keep the generic one and introduce a 2nd "entity" argument?
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
2 participants
Related issue: Resolves #41565
Surfaces the new include_all label scope on the policy and report (query) edit forms via a "Custom" target dropdown.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Summary by CodeRabbit
New Features
Tests