Policies: Include/Exclude label targeting on edit policy page (#33441) - #47354
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #47354 +/- ##
========================================
Coverage 67.19% 67.20%
========================================
Files 3274 3275 +1
Lines 227979 227976 -3
Branches 11751 11891 +140
========================================
+ Hits 153200 153201 +1
+ Misses 60963 60958 -5
- Partials 13816 13817 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
nulmete
marked this pull request as ready for review
June 10, 2026 19:00
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
WalkthroughThis PR adds "exclude all labels" support to the policies feature through a dedicated label-targeting hook. It consolidates the label-configuration type ( 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 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
frontend/pages/policies/hooks/usePolicyLabelTargets.tests.tsx (2)
49-163: ⚖️ Poor tradeoffConsider expanding test coverage for completeness.
The test suite covers the main happy paths effectively, but several scenarios remain untested:
- Mode switching: Toggling between
anyandallmodes viaincludeConfig.onSelectMode/excludeConfig.onSelectMode- Deselecting labels: Setting
value: falseinonSelectLabel- Additional seeding cases:
excludeAnyandincludeAllprop initialization- Error and loading states: Verifying
isLoadingLabelsandisErrorLabelspropagation- Combined selections: Include + exclude labels selected simultaneously
These additions would improve confidence in edge cases, but the existing coverage is sufficient for the main flows.
🤖 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/hooks/usePolicyLabelTargets.tests.tsx` around lines 49 - 163, Add unit tests for untested scenarios in usePolicyLabelTargets: cover mode switching by calling selectorProps.includeConfig.onSelectMode and selectorProps.excludeConfig.onSelectMode and asserting includeConfig.mode / excludeConfig.mode and resulting getLabelsPayload; test deselecting labels by invoking selectorProps.includeConfig.onSelectLabel / excludeConfig.onSelectLabel with value: false and confirming payload updates; add seeding tests for includeAll and excludeAny by initializing usePolicyLabelTargets with includeAll and excludeAny props and asserting selectedTargetType, hasCustomLabels, and getLabelsPayload; add tests that simulate loading and error label states by asserting isLoadingLabels and isErrorLabels on selectorProps (mock the labelSummariesHandler accordingly); and add a combined selection test that selects both include and exclude labels and asserts the combined getLabelsPayload.
82-89: ⚡ Quick winConsider using
waitForinstead ofsetTimeoutfor the non-fetch test.The test uses
setTimeout(resolve, 200)to verify that labels are not fetched when no team is selected. This approach is fragile in slow CI environments where 200ms might not be sufficient, and it adds unnecessary delay to the test suite.♻️ Proposed fix using query state inspection
Instead of waiting an arbitrary duration, inspect the query state directly:
it("does not fetch labels when no team is selected", async () => { const { result } = renderHook(() => usePolicyLabelTargets(), { wrapper: buildWrapper({ isPremiumTier: true, currentTeam: undefined }), }); - await new Promise((resolve) => setTimeout(resolve, 200)); - expect(result.current.selectorProps.labels).toEqual([]); + // Query should be disabled, so no loading state + expect(result.current.selectorProps.isLoadingLabels).toBe(false); + expect(result.current.selectorProps.labels).toEqual([]); });This is faster and more deterministic.
🤖 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/hooks/usePolicyLabelTargets.tests.tsx` around lines 82 - 89, The test in usePolicyLabelTargets ("does not fetch labels when no team is selected") uses a fixed setTimeout which is flaky; replace the arbitrary delay with testing-library's waitFor to await the expected stable condition: import waitFor from `@testing-library/react`, remove await new Promise(...), and use await waitFor(() => expect(result.current.selectorProps.labels).toEqual([])); locate the test that calls renderHook(() => usePolicyLabelTargets(), { wrapper: buildWrapper({ isPremiumTier: true, currentTeam: undefined }) }) and update it accordingly so it deterministically waits for selectorProps.labels to be empty.frontend/pages/policies/hooks/usePolicyLabelTargets.tsx (1)
155-167: ⚡ Quick winConsider guarding against unstable prop references.
The
useEffectreseeds all state wheneverincludeAny,includeAll,excludeAny, orexcludeAllchange identity. If a parent component passes new arrays with identical content on re-render, user selections will be lost. The test comments indicate this is addressed in production (PolicyContext provides stable references), but the hook doesn't enforce this contract.Run the following script to verify all consumers pass stable references from PolicyContext:
#!/bin/bash # Description: Verify that usePolicyLabelTargets consumers pass stable label props from PolicyContext # Search for usages of the hook ast-grep --pattern 'usePolicyLabelTargets($$$)'If any consumer constructs label arrays inline (e.g.,
usePolicyLabelTargets({ includeAny: [...] })), the arrays won't be stable across renders.🛡️ Optional deep-equality guard
If non-PolicyContext consumers are found, consider using a deep-equality check in the dependency array:
+ import { isEqual } from "lodash"; + + const [prevLabels, setPrevLabels] = useState({ includeAny, includeAll, excludeAny, excludeAll }); + useEffect(() => { + const current = { includeAny, includeAll, excludeAny, excludeAll }; + if (isEqual(prevLabels, current)) return; + setPrevLabels(current); + const seed = derivePolicyTargetState({ includeAny, includeAll, excludeAny, excludeAll, }); setSelectedTargetType(seed.targetType); setIncludeMode(seed.includeMode); setIncludeLabels(seed.includeLabels); setExcludeMode(seed.excludeMode); setExcludeLabels(seed.excludeLabels); - }, [includeAny, includeAll, excludeAny, excludeAll]); + }, [includeAny, includeAll, excludeAny, excludeAll, prevLabels]);However, this adds complexity and a lodash dependency, so only apply if instability is observed in practice.
🤖 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/hooks/usePolicyLabelTargets.tsx` around lines 155 - 167, The effect in usePolicyLabelTargets reseeds state whenever the identity of includeAny/includeAll/excludeAny/excludeAll changes, which will wipe user selections if callers pass new-but-equal arrays; modify the effect to guard against unstable prop references by comparing the previous and current label arrays by deep equality before reseeding (i.e., compute a stable "seed" only when the contents actually differ). Concretely, add a small equality check around the derivePolicyTargetState call inside the existing useEffect in usePolicyLabelTargets (or replace the effect with a deep-compare effect), and only call setSelectedTargetType / setIncludeMode / setIncludeLabels / setExcludeMode / setExcludeLabels when the deep-equality check shows the inputs truly changed; you can implement the comparison with a lightweight helper (deep equality or JSON.stringify) or use lodash.isEqual if available.
🤖 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.
Nitpick comments:
In `@frontend/pages/policies/hooks/usePolicyLabelTargets.tests.tsx`:
- Around line 49-163: Add unit tests for untested scenarios in
usePolicyLabelTargets: cover mode switching by calling
selectorProps.includeConfig.onSelectMode and
selectorProps.excludeConfig.onSelectMode and asserting includeConfig.mode /
excludeConfig.mode and resulting getLabelsPayload; test deselecting labels by
invoking selectorProps.includeConfig.onSelectLabel / excludeConfig.onSelectLabel
with value: false and confirming payload updates; add seeding tests for
includeAll and excludeAny by initializing usePolicyLabelTargets with includeAll
and excludeAny props and asserting selectedTargetType, hasCustomLabels, and
getLabelsPayload; add tests that simulate loading and error label states by
asserting isLoadingLabels and isErrorLabels on selectorProps (mock the
labelSummariesHandler accordingly); and add a combined selection test that
selects both include and exclude labels and asserts the combined
getLabelsPayload.
- Around line 82-89: The test in usePolicyLabelTargets ("does not fetch labels
when no team is selected") uses a fixed setTimeout which is flaky; replace the
arbitrary delay with testing-library's waitFor to await the expected stable
condition: import waitFor from `@testing-library/react`, remove await new
Promise(...), and use await waitFor(() =>
expect(result.current.selectorProps.labels).toEqual([])); locate the test that
calls renderHook(() => usePolicyLabelTargets(), { wrapper: buildWrapper({
isPremiumTier: true, currentTeam: undefined }) }) and update it accordingly so
it deterministically waits for selectorProps.labels to be empty.
In `@frontend/pages/policies/hooks/usePolicyLabelTargets.tsx`:
- Around line 155-167: The effect in usePolicyLabelTargets reseeds state
whenever the identity of includeAny/includeAll/excludeAny/excludeAll changes,
which will wipe user selections if callers pass new-but-equal arrays; modify the
effect to guard against unstable prop references by comparing the previous and
current label arrays by deep equality before reseeding (i.e., compute a stable
"seed" only when the contents actually differ). Concretely, add a small equality
check around the derivePolicyTargetState call inside the existing useEffect in
usePolicyLabelTargets (or replace the effect with a deep-compare effect), and
only call setSelectedTargetType / setIncludeMode / setIncludeLabels /
setExcludeMode / setExcludeLabels when the deep-equality check shows the inputs
truly changed; you can implement the comparison with a lightweight helper (deep
equality or JSON.stringify) or use lodash.isEqual if available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 050fc649-8ea4-47e9-81c9-30ae8910cd39
📥 Commits
Reviewing files that changed from the base of the PR and between f308fec and 5eb8d4950ce3461770ac90ccfd002c3c6444194d.
📒 Files selected for processing (15)
frontend/components/TargetLabelSelector/TargetLabelSelector.tests.tsxfrontend/components/TargetLabelSelector/TargetLabelSelector.tsxfrontend/components/TargetLabelSelector/index.tsfrontend/context/policy.tsxfrontend/pages/ManageControlsPage/OSSettings/cards/ConfigurationProfiles/components/ProfileUploader/components/AddProfileModal/AddProfileModal.tsxfrontend/pages/policies/constants.tsfrontend/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.tests.tsxfrontend/pages/policies/edit/components/SaveNewPolicyModal/SaveNewPolicyModal.tsxfrontend/pages/policies/edit/screens/QueryEditor.tsxfrontend/pages/policies/hooks/index.tsfrontend/pages/policies/hooks/usePolicyLabelTargets.tests.tsxfrontend/pages/policies/hooks/usePolicyLabelTargets.tsx
nulmete
commented
Jun 10, 2026
| getLabelsPayload: () => ReturnType<typeof buildPolicyLabelsPayload>; | ||
| } | ||
|
|
||
| const usePolicyLabelTargets = ({ |
Member
Author
There was a problem hiding this comment.
hook used both in Save and Edit forms, added for DRY purposes.
juan-fdz-hawa
previously approved these changes
Jun 11, 2026
Base automatically changed from
33441-policies-save-modal-custom-targets
to
main
June 11, 2026 13:00
Introduce usePolicyLabelTargets hook (owns label fetch, controlled state, and edit-init) and migrate PolicyForm + SaveNewPolicyModal onto the tabbed TargetLabelSelector. Plumb labels_exclude_all through PolicyContext, the edit-page load, and the create payload.
Add tests for mode switching, label deselection, include_all/exclude_any seeding, combined include+exclude payload, and loading/error states. Replace the flaky setTimeout in the no-team test with a deterministic labelsAPI.summary spy.
nulmete
force-pushed
the
33441-policies-edit-custom-targets
branch
from
June 11, 2026 13:05
1250c38 to
244dd1b
Compare
juan-fdz-hawa
approved these changes
Jun 11, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Policies create/edit UI to support the newer tabbed include/exclude label targeting experience (and centralizes that logic in a reusable hook), while also refactoring TargetLabelSelector’s prop/types naming to match the new config shape.
Changes:
- Added
usePolicyLabelTargetshook to own label fetching, controlled selector state, and mapping UI selections to policy API payload fields. - Refactored policy create/edit flows to use the tabbed
TargetLabelSelectorand to plumblabels_exclude_allthrough form/context payloads. - Renamed
TargetLabelSelectorinclude/exclude config types/props (ILabelTabConfig→ILabelConfig,include/exclude→includeConfig/excludeConfig) and updated call sites + tests.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| frontend/pages/policies/hooks/usePolicyLabelTargets.tsx | New hook: fetches label summaries, manages include/exclude state, builds policy labels payload. |
| frontend/pages/policies/hooks/usePolicyLabelTargets.tests.tsx | Unit tests covering seeding, payload mapping, and label fetch states for the new hook. |
| frontend/pages/policies/hooks/index.ts | Exposes usePolicyLabelTargets (default export + types) for policy pages. |
| frontend/pages/policies/edit/screens/QueryEditor.tsx | Includes labels_exclude_all when constructing the create-policy payload. |
| frontend/pages/policies/edit/EditPolicyPage.tsx | Stores labels_exclude_all from the loaded policy into PolicyContext. |
| frontend/pages/policies/edit/components/SaveNewPolicyModal/SaveNewPolicyModal.tsx | Refactors modal to use usePolicyLabelTargets and new selector props; assigns labels payload via hook. |
| frontend/pages/policies/edit/components/SaveNewPolicyModal/SaveNewPolicyModal.tests.tsx | Updates tests to align with hook-driven label fetching and new payload fields. |
| frontend/pages/policies/edit/components/PolicyForm/PolicyForm.tsx | Refactors policy edit form to use usePolicyLabelTargets + tabbed selector; simplifies save-disabled logic. |
| frontend/pages/policies/edit/components/PolicyForm/PolicyForm.tests.tsx | Updates/extends tests for new selector behavior (including exclude tab preload and combined include+exclude). |
| frontend/pages/policies/constants.ts | Adds shared empty-state description constant for policy targeting selector. |
| frontend/pages/ManageControlsPage/OSSettings/.../AddProfileModal.tsx | Updates selector prop/type names to the new includeConfig/excludeConfig + ILabelConfig. |
| frontend/context/policy.tsx | Adds lastEditedQueryLabelsExcludeAll to context state and setter plumbing. |
| frontend/components/TargetLabelSelector/TargetLabelSelector.tsx | Renames config types/props to ILabelConfig/includeConfig/excludeConfig and wires through the component. |
| frontend/components/TargetLabelSelector/TargetLabelSelector.tests.tsx | Updates selector tests for renamed props/types. |
| frontend/components/TargetLabelSelector/index.ts | Updates exported type name to ILabelConfig. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+196
to
+221
| const excludeConfig: ILabelConfig = { | ||
| selectedLabels: excludeLabels, | ||
| onSelectLabel: ({ name, value }) => | ||
| setExcludeLabels((prev) => ({ ...prev, [name]: value })), | ||
| showModeToggle: true, | ||
| mode: excludeMode, | ||
| onSelectMode: setExcludeMode, | ||
| anyTooltip: ( | ||
| <> | ||
| Will not target hosts that have{" "} | ||
| <em> | ||
| <b>any</b> | ||
| </em>{" "} | ||
| of these labels. | ||
| </> | ||
| ), | ||
| allTooltip: ( | ||
| <> | ||
| Will not target hosts that have{" "} | ||
| <em> | ||
| <b>all</b> | ||
| </em>{" "} | ||
| of these labels. | ||
| </> | ||
| ), | ||
| }; |
3 tasks
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
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.
Related issue: Resolves #46583
Figma: https://www.figma.com/design/0F1sw63SuYaKVWlcL7mnc6/-33441-Policies--Custom-targets-with-%22Include-any%22-and-%22Exclude-any%22?node-id=5319-2300&t=Fszpf83KhcZ7ViWh-0
Testing
Added/updated automated tests
QA'd all new/changed functionality manually
Note: selecting
Exclude Allwill fail since BE doesn't support it yet. Will be tackled as part of #46582.Screen.Recording.2026-06-10.at.3.57.56.PM.mov
Summary by CodeRabbit
New Features
Refactor