chore: improve indicator for non system records with empty notes Bed 8922 - #3000
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesNoteCell note handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
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: 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/History/NoteCell.tsx`:
- Around line 35-39: Update the no-note placeholder in the NoteCell rendering
branch to add tabIndex={0} to the <p> element wrapped by Tooltip, making the “No
notes” tooltip trigger keyboard-accessible while preserving the existing
placeholder content and styling.
🪄 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: 99df937d-549b-435a-a46f-dd567b0c1c47
📒 Files selected for processing (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/NoteCell.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/NoteCell.tsx
LucasParraF
added
the
user interface
| {actor === BloodHoundString ? ( | ||
| <Tooltip tooltip={`No notes for ${BloodHoundString} history`}> | ||
| {!note ? ( | ||
| <Tooltip tooltip={noNoteTooltip}> |
There was a problem hiding this comment.
Could we potentially move this tooltip to wrap the entire cell div instead? the <p> tag itself is only as wide as the dash character and only triggers when the mouse is directly on top, which feels a little too difficult to trigger
There was a problem hiding this comment.
agreed and that way only use Tooltip component once.
|
|
||
| const noteIsActive = selected?.id === id; | ||
| const tip = !note ? 'No notes' : noteIsActive ? 'Hide note' : 'Show note'; | ||
| const tip = noteIsActive ? 'Hide note' : 'Show note'; |
There was a problem hiding this comment.
nit(ish): Maybe we can make tip two words and will it help make it more descriptive.
| const noteToolTipContent = note | ||
| ? noteIsActive | ||
| ? 'Hide note' | ||
| : 'Show note' | ||
| : actor === BloodHoundString | ||
| ? `No notes for ${BloodHoundString} history` | ||
| : 'No notes'; |
There was a problem hiding this comment.
Thank you for the changes, wonder if we could benefit with readability by breaking this done further. Maybe creating a variable for the first condition and for the second and you then end up with a one line terniary:
const activeNoteTooltipContent = noteIsActive ? 'Hide note' : 'Show note';
const isSystemNote = actor === BloodHoundString;
const systemNoteTooltipContent = isSystemNote ? `No notes for ${BloodHoundString} history` : 'No notes';
const noteTooltipContent = note ? activeNoteTooltipContent : systemNoteTooltipContent
maffkipp
left a comment
maffkipp
left a comment
There was a problem hiding this comment.
works great, appreciate the change!
specter-flq
left a comment
specter-flq
left a comment
There was a problem hiding this comment.
LGTM! Thank you for the changes!
3 participants
Description
Updated the History table to change how the absence of notes is displayed for records not created by BloodHound. Instead of rendering a disabled note icon, the table now displays a - character. The tooltip behavior was also updated to provide appropriate messaging depending on whether the action was performed by BloodHound or by another source.
Motivation and Context
Resolves BED-8922
How Has This Been Tested?
Tested locally and added new test cases to cover the changes
Screenshots (optional):
Video with demo attached to the Jira ticket
Types of changes
Checklist:
Summary by CodeRabbit
Summary
userEventand added/adjusted tooltip coverage for missing-note scenarios.