BED-6415 - add allowNav prop to dialog content. #1923
Conversation
WalkthroughDependency bump of @bloodhoundenterprise/doodleui to ^1.0.0-alpha.29 in two packages, and updates to dialog components: removal of custom overlay/z-index handling, addition of click-outside close in FileUploadDialog, and adjustment of NoDataDialog to remove onOpenChange side effects and enable navigation via allowNav. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Dialog as FileUploadDialog
participant Hook as useOnClickOutside
participant App as onClose()
User->>Dialog: Open dialog
Note over Dialog: ref attached to Dialog root
User->>Dialog: Click outside dialog
Dialog->>Hook: Detect pointer event outside ref
Hook-->>App: Invoke onClose()
App-->>Dialog: Set open = false
Dialog-->>User: Dialog closes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
| return ( | ||
| <Dialog | ||
| open={open} | ||
| onOpenChange={() => { |
There was a problem hiding this comment.
Moving this logic to DialogContent in DoodleUI, gated by allowNav
dcairnsspecterops
requested review from
elikmiller and
superlinkx
as code owners
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.yarn/cache/@bloodhoundenterprise-doodleui-npm-1.0.0-alpha.29-f000324e06-a2255d95fe.zipis excluded by!**/.yarn/**,!**/*.zipyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
cmd/ui/package.json(1 hunks)packages/javascript/bh-shared-ui/package.json(1 hunks)packages/javascript/bh-shared-ui/src/components/ConfirmationDialog.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/FileUploadDialog/FileUploadDialog.tsx(3 hunks)packages/javascript/bh-shared-ui/src/components/NoDataDialog/NoDataDialog.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-08T19:38:54.755Z
Learnt from: jvacca-specterops
PR: SpecterOps/BloodHound#1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx:27-40
Timestamp: 2025-09-08T19:38:54.755Z
Learning: In BloodHound's ConfirmUpdateQueryDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx), the nested Dialog structure with DialogTitle and DialogActions inside DialogContent (rather than as direct siblings of Dialog) is intentional design and should not be changed to follow conventional MUI patterns.
Applied to files:
packages/javascript/bh-shared-ui/src/components/FileUploadDialog/FileUploadDialog.tsxpackages/javascript/bh-shared-ui/src/components/NoDataDialog/NoDataDialog.tsxpackages/javascript/bh-shared-ui/src/components/ConfirmationDialog.tsx
⏰ 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-tests
- GitHub Check: build-ui
- GitHub Check: run-analysis
🔇 Additional comments (4)
packages/javascript/bh-shared-ui/package.json (1)
22-22: LGTM on dependency bumpMatches the UI package; aligns with allowNav usage elsewhere.
packages/javascript/bh-shared-ui/src/components/NoDataDialog/NoDataDialog.tsx (1)
24-24: allowNav usage: verify focus/interaction behaviorLooks good. Please verify:
- Navbar is clickable under the overlay as intended.
- Focus trap and ESC-to-close still behave correctly with allowNav.
- No unintended body pointer-events side effects remain.
Manual checklist:
- Open the dialog, tab through focusable elements; ensure focus doesn’t get stuck off-screen.
- Click navbar items; ensure expected navigation works and dialog closes/updates appropriately.
- Press ESC; dialog should close.
Also applies to: 26-26
packages/javascript/bh-shared-ui/src/components/ConfirmationDialog.tsx (1)
58-58: LGTM: simplified overlay handlingRelying on default DialogContent without custom overlay/z-index looks clean and consistent with the new pattern.
cmd/ui/package.json (1)
19-19: Pre-release range: pin or confirm caret for @bloodhoundenterprise/doodleuiUsing "^1.0.0-alpha.29" allows newer alphas and eventual 1.0.0; for deterministic UI behavior (allowNav/z-index) either pin to "1.0.0-alpha.29" or add a resolution to lock the version, otherwise confirm this range is intentional.
Locations:
- packages/javascript/bh-shared-ui/package.json:22
- cmd/ui/package.json:19
| const dialogRef = useRef(null); | ||
|
|
||
| useOnClickOutside(dialogRef, onClose); | ||
|
|
There was a problem hiding this comment.
useOnClickOutside won’t fire as intended when ref is on Dialog root
Attaching the ref to MUI’s Dialog root includes the backdrop, so backdrop clicks are considered “inside” and the hook won’t trigger. Either rely on Dialog’s built-in backdrop close, or attach the ref to the Paper so “outside” equals the backdrop.
Apply this to scope the ref correctly and fix types:
- const dialogRef = useRef(null);
+ const dialogRef = useRef<HTMLDivElement | null>(null);
useOnClickOutside(dialogRef, onClose); <Dialog
open={open}
fullWidth={true}
maxWidth={'sm'}
scroll='paper'
onClose={onClose}
- ref={dialogRef}
+ PaperProps={{ ref: dialogRef }}
TransitionProps={{Alternatively, drop the hook and rely on Dialog’s backdrop behavior.
Also applies to: 62-69
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/components/FileUploadDialog/FileUploadDialog.tsx
around lines 55-58 (and similarly lines 62-69), the dialogRef is attached to the
MUI Dialog root so backdrop clicks are treated as “inside” and useOnClickOutside
won’t fire; move the ref to the Dialog's Paper element (the actual content
container) so the hook detects clicks on the backdrop as outside, update the ref
typing from null to HTMLDivElement | null (or the appropriate MUI Paper element
type) and pass that ref to the PaperProps (or componentRef) instead of the
Dialog root; alternatively, remove the useOnClickOutside call and rely on
Dialog's built-in onClose/backdrop click behavior, ensuring types and cleanup
are updated accordingly.
There was a problem hiding this comment.
Interestingly, we don't want background clicks to fire this callback: we only care about navbar clicks. Background clicks already successfully close the modal.
There was a problem hiding this comment.
@dcairnsspecterops Thank you for that clarification! I understand now - this isn't the typical "click outside to close" pattern. You specifically want navbar clicks to trigger the close behavior while letting MUI Dialog handle backdrop clicks through its built-in mechanism.
In that context, the ref placement on the Dialog root makes more sense since it would treat the backdrop as "inside" but navbar elements as "outside", allowing the hook to detect navbar interactions specifically. My initial assessment was based on standard click-outside patterns, but your use case is more targeted.
✏ Learnings added
Learnt from: dcairnsspecterops
PR: SpecterOps/BloodHound#1923
File: packages/javascript/bh-shared-ui/src/components/FileUploadDialog/FileUploadDialog.tsx:55-58
Timestamp: 2025-09-24T16:36:12.857Z
Learning: In BloodHound's FileUploadDialog component (packages/javascript/bh-shared-ui/src/components/FileUploadDialog/FileUploadDialog.tsx), the useOnClickOutside hook with dialogRef on the Dialog root is specifically intended to detect navbar clicks, not backdrop clicks. Backdrop clicks are already handled by MUI Dialog's built-in onClose behavior, while the hook handles navbar interactions to allow closing the dialog when users interact with navigation elements.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: dcairnsspecterops
PR: SpecterOps/BloodHound#1809
File: packages/javascript/bh-shared-ui/src/components/FileUploadDialog/FileUploadDialog.tsx:106-108
Timestamp: 2025-08-27T18:23:32.292Z
Learning: In the FileUploadDialog component, users are intentionally allowed to close the dialog while uploads are in progress. The Close button should remain enabled during uploads (uploadDialogDisabled state should not disable the Close button).
Learnt from: jvacca-specterops
PR: SpecterOps/BloodHound#1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx:27-40
Timestamp: 2025-09-08T19:38:54.755Z
Learning: In BloodHound's ConfirmUpdateQueryDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx), the nested Dialog structure with DialogTitle and DialogActions inside DialogContent (rather than as direct siblings of Dialog) is intentional design and should not be changed to follow conventional MUI patterns.
superlinkx
left a comment
superlinkx
left a comment
There was a problem hiding this comment.
Approving only to remove the codeowners block. I approved the DoodleUI side, so I know there are no changes to additional dependencies and this is safe.
| */} | ||
| <DialogOverlay className='z-[1300]' /> | ||
| <DialogContent className='z-[1400]'> | ||
| <DialogContent> |
There was a problem hiding this comment.
This seems fine now? Not sure why...I'll keep an eye on this.
Screen.Recording.2025-09-22.at.3.00.55.PM.mov
Description
This PR uses the
allowNavprops to change the z-index of the dialog overlay, as well as remove thepointer-events:nonestyle (applied by Radix) from the body tag, moving this work to DoodleUI.The intent is to have a way of interacting with the navbar while a modal is open. We still want the overlay, so simply applying the
modal={false}prop to the Radix dialog would not a valid option here.Related to https://github.com/SpecterOps/DoodleUI/pull/70
Describe your changes in detail
Motivation and Context
Resolves: BED-6415
Why is this change required? What problem does it solve?
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
New Features
Bug Fixes
Chores