refactor: Change error message when the user do not run a cypher before saving a rule BED-6921 #2177
Conversation
WalkthroughAdded an internal helper to centralize API error-message mapping for privilege-zone save error handling, refactored handleError to use it (including a specific "seeds are required" case), and added unit tests covering multiple Axios-like error shapes and notification outcomes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
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 |
LucasParraF
added
the
user interface
LucasParraF
changed the title
LucasParraF
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.ts (1)
44-44: Minor: Consider consistent capitalization of "Cypher".The term "cypher" appears lowercase twice in this message. Cypher is typically capitalized as a proper noun (the query language name). Consider updating for consistency.
- message = `To save a ${entity} created using cypher, the cypher must be run first. Click "Update Sample Results" to continue`; + message = `To save a ${entity} created using Cypher, the Cypher query must be run first. Click "Update Sample Results" to continue`;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts:184-187
Timestamp: 2025-08-27T21:15:32.207Z
Learning: In the BloodHound codebase, syntax fixes for invalid Cypher patterns (like `*..` → `*1..`) may be considered out of scope for specific PRs, even when flagged during review.
📚 Learning: 2025-09-08T19:01:53.112Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx:108-148
Timestamp: 2025-09-08T19:01:53.112Z
Learning: In BloodHound's CypherSearch component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx), the sharing state reset for sharedIds and isPublic after the two-step permissions update is handled elsewhere in the codebase, so additional state reset callbacks in the updateQueryPermissions function are not needed.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.ts
🔇 Additional comments (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.ts (1)
38-50: Button label and error handling verified.The button label "Update Sample Results" is correctly referenced and exists in the codebase. The error handling code defensively manages unknown API messages by falling back to a generic error message if the API response doesn't match the expected cases. The code is sound.
| switch (apiMessage) { | ||
| case 'name must be unique': | ||
| message = `Error ${action} ${entity}: ${entity} names must be unique. Please provide a unique name for your new ${entity} and try again.`; | ||
| break; | ||
|
|
||
| case 'seeds are required': | ||
| message = `To save a ${entity} created using cypher, the cypher must be run first. Click "Update Sample Results" to continue`; | ||
| break; | ||
|
|
||
| default: | ||
| message = `An unexpected error occurred while ${action} the ${entity}. Message: ${apiMessage}. Please try again.`; | ||
| break; |
There was a problem hiding this comment.
Could we make this switch statement logic a small function and add a unit test for it please? Something like apiErrorMessageToDisplay
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.test.ts (1)
41-41: Consider a more specific type for improved type safety.The
anytype works for testing purposes but could be replaced with a more specific type using TypeScript's utility types.Consider this alternative:
-// 'any' type used as an easy way to make errors optional -const mockAxiosStatusTextOnly: any = cloneDeep(mockAxiosError); +// Create a mock without the errors array to test statusText fallback +const mockAxiosStatusTextOnly = cloneDeep(mockAxiosError) as typeof mockAxiosError & { + response: Omit<typeof mockAxiosError.response, 'data'> & { + data: Partial<typeof mockAxiosError.response.data>; + }; +};However, given this is test code and the intent is clear, the current approach is acceptable.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.test.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts:184-187
Timestamp: 2025-08-27T21:15:32.207Z
Learning: In the BloodHound codebase, syntax fixes for invalid Cypher patterns (like `*..` → `*1..`) may be considered out of scope for specific PRs, even when flagged during review.
🧬 Code graph analysis (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.test.ts (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.ts (1)
handleError(33-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: build-ui
- GitHub Check: run-tests
🔇 Additional comments (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.test.ts (2)
47-56: LGTM!The test constants are well-structured and align with the expected notification format from the
handleErrorimplementation.
58-114: Excellent test coverage for the new Cypher error handling!The test suite comprehensively covers multiple error scenarios including the new feature for PR BED-6921. The Cypher-specific test (lines 92-103) correctly validates that when
seeds are requiredis returned by the API, users receive actionable guidance: "To save a rule created using Cypher, the Cypher must be run first."All test cases properly verify the notification callback parameters (message, key, options) and follow a consistent, maintainable pattern.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.test.ts
Outdated
Show resolved
Hide resolved
urangel
left a comment
urangel
left a comment
There was a problem hiding this comment.
Thank you for including these tests!
urangel
deleted the
BED-6921-Change-error-message-when-the-user-do-not-run-a-cypher-before-saving-a-rule
branch
Description
On Privilege Zone, when a user is creating a rule using cypher and they attempt to move forward without running the cypher query first, we are displaying the following message "An unexpected error occurred while creating the rule. Message: seeds are required. Please try again.". We want to change the verbiage displayed to be more clear to the user on what should be done.
Motivation and Context
Resolves https://specterops.atlassian.net/browse/BED-6921
How Has This Been Tested?
Tested locally
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.