feat: PZ Rule Creation: Sample results/query should not clear when user changes rule type [BED-8303]#2904
Conversation
|
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:
📝 WalkthroughWalkthrough
ChangesSeed Retention on Rule-Type Toggle
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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
added
the
user interface
![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
🤖 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
`@packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/RuleForm/RuleForm.tsx`:
- Around line 133-147: The issue is that when toggling rule types, the code
restores seeds from prevSeeds but always clears selectedObjects to an empty
array, causing a mismatch between displayed selections and the actual payload.
Similar to how prevSeeds preserves user work across toggles, you need to also
preserve and restore selectedObjects. Add a prevSelectedObjects field to the
state, and when swapping rule types, swap selectedObjects with
prevSelectedObjects just as you do with seeds and prevSeeds. This ensures that
when returning to ObjectID rule type, the previously selected objects are
restored along with their corresponding seeds.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: ec2651db-0901-4b88-9149-9f6cfd79d2f6
📒 Files selected for processing (3)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/RuleForm/RuleForm.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/RuleForm/SeedSelection.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/RuleForm/types.ts
💤 Files with no reviewable changes (1)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/RuleForm/SeedSelection.tsx
| * when we introduce a third rule type, simple toggling will be insufficient | ||
| * and we will need to store all 3 seed types explicitly. | ||
| * That's because we reuse seeds state for _both_ rule types, | ||
| * even though we store `selectedObjects` separately |
There was a problem hiding this comment.
It might be worth clarifying in this comment that cypher queries will not be preserved if "Run" is not clicked first.
Also minor typo: Tthis on line 141
|
Would it be possible to add a test covering this behavior in |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/RuleForm/RuleForm.test.tsx`:
- Around line 486-488: The test selector for the Cypher textbox relies on
className matching which is brittle and will break if styling changes. Add a
data-testid attribute to the textbox wrapper in the PrivilegeZonesCypherEditor
component (e.g., 'privilege-zones_cypher-editor-input'), then replace all
occurrences of the className-based selection pattern that retrieves the textbox
with screen.getByTestId() or screen.queryByTestId() using the new test ID for a
stable, functional selector that is decoupled from CSS class names.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 6ab8a0c4-5a49-4daa-8132-8418b10c2db6
📒 Files selected for processing (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/RuleForm/RuleForm.test.tsx
| const textBoxes = screen.getAllByRole('textbox'); | ||
| const cypherTextBox = textBoxes.find((box) => box.className === 'flex-1'); | ||
| await user.click(cypherTextBox!); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether a stable selector already exists for the cypher editor input.
rg -n "className === 'flex-1'" packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/RuleForm/RuleForm.test.tsx
rg -n "PrivilegeZonesCypherEditor" packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/RuleForm -g "*.tsx"
rg -n "data-testid|aria-label|label|role=" packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/RuleForm -g "*.tsx"Repository: SpecterOps/BloodHound
Length of output: 6600
🏁 Script executed:
fd -t f "PrivilegeZonesCypherEditor" packages/javascript/bh-shared-ui/src/views/PrivilegeZones --maxdepth 3Repository: SpecterOps/BloodHound
Length of output: 396
🏁 Script executed:
cat -n packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesCypherEditor.tsx | head -100Repository: SpecterOps/BloodHound
Length of output: 280
🏁 Script executed:
cat -n packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesCypherEditor/PrivilegeZonesCypherEditor.tsxRepository: SpecterOps/BloodHound
Length of output: 8670
🏁 Script executed:
cat -n packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/RuleForm/RuleForm.test.tsx | sed -n '480,495p'Repository: SpecterOps/BloodHound
Length of output: 951
🏁 Script executed:
cat -n packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/RuleForm/RuleForm.test.tsx | sed -n '360,370p; 395,405p; 545,555p'Repository: SpecterOps/BloodHound
Length of output: 1574
Add a stable selector for the Cypher textbox instead of relying on class-name matching.
The pattern box.className === 'flex-1' at lines 365, 400, 487, and 551 is style-coupled and will break on non-functional UI changes. The PrivilegeZonesCypherEditor component already exposes role='textbox' on the editor wrapper; consider adding a dedicated data-testid (e.g., data-testid='privilege-zones_cypher-editor-input') to the textbox wrapper in the component and querying it directly in tests, or using a more specific role-based selector combined with accessible naming.
🤖 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
`@packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/RuleForm/RuleForm.test.tsx`
around lines 486 - 488, The test selector for the Cypher textbox relies on
className matching which is brittle and will break if styling changes. Add a
data-testid attribute to the textbox wrapper in the PrivilegeZonesCypherEditor
component (e.g., 'privilege-zones_cypher-editor-input'), then replace all
occurrences of the className-based selection pattern that retrieves the textbox
with screen.getByTestId() or screen.queryByTestId() using the new test ID for a
stable, functional selector that is decoupled from CSS class names.
dcairnsspecterops
changed the title
dcairnsspecterops
deleted the
BED-8303-rule-state-switching-cypher-object-id
branch
2 participants
Description
PZ-rule-type-change.mov
Motivation and Context
Resolves BED-8303
Users do not like losing their work when changing rule types during PX rule creation. We should store their work between rule type changes.
Therefore, this PR changes the logic when:
Previously: their sample results and query would be lost, since we did not store seeds in the context between rule type changes.
Now: we will store the sample results in
prevSeeds, and use this value to hydrate the rule form. When the user changes rule type, their seeds (which contain queries, etc) will be maintained. This will clear out when the user leaves the rule form.How Has This Been Tested?
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
Bug Fixes
Tests