feat: Keyboard Shortcuts BED-5787 #2132
Open
Conversation
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
…d into POC-command-palette
Contributor
WalkthroughReplaces the single FileUploadDialog provider with a DialogProviders wrapper, adds a KeyboardShortcutsDialog and ShortcutSection, introduces a useKeybindings hook and keyboard-shortcuts context/provider, and wires Alt+key shortcuts and autofocus/ref focus handling across multiple UI components. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant KeyHandler as useKeybindings
participant Context as KBDialogContext
participant App
participant Dialog as KeyboardShortcutsDialog
User->>Browser: Press Alt+H
Browser->>KeyHandler: keydown (Alt+H)
KeyHandler->>Context: setShowKeyboardShortcutsDialog(true)
Context->>App: context value updated
App->>Dialog: render open=true
Dialog->>User: displays shortcuts
User->>Dialog: Press Escape
Dialog->>Context: setShowKeyboardShortcutsDialog(false)
Context->>App: context value updated
App->>Dialog: render open=false
sequenceDiagram
participant User
participant Browser
participant KeyHandler as useKeybindings
participant Router
participant GraphView
User->>Browser: Press KeyC
Browser->>KeyHandler: keydown (C)
KeyHandler->>GraphView: set exploreSearchTab = 'cypher'
GraphView->>User: shows Cypher tab
User->>Browser: Press KeyI
Browser->>KeyHandler: keydown (I)
KeyHandler->>GraphView: request selection change (reads previousSelectedItem)
GraphView->>GraphView: compute next selection
GraphView->>User: selection updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (7)📚 Learning: 2025-09-08T19:38:54.755ZApplied to files:
📚 Learning: 2025-08-27T19:58:12.996ZApplied to files:
📚 Learning: 2025-08-27T19:22:50.905ZApplied to files:
📚 Learning: 2025-09-08T19:01:53.112ZApplied to files:
📚 Learning: 2025-08-27T19:48:23.432ZApplied to files:
📚 Learning: 2025-11-06T21:35:45.118ZApplied to files:
📚 Learning: 2025-11-06T21:23:30.689ZApplied to files:
⏰ 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)
🔇 Additional comments (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 |
dcairnsspecterops
changed the title
dcairnsspecterops
changed the title
dcairnsspecterops
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/PathfindingSearch.tsx (1)
69-69: Consider simplifying the boolean expression.The double negation
!!is unnecessary here since the conditionsourceSearchTerm && !destinationSearchTermalready evaluates to a boolean-compatible value that React will handle correctly.🔎 Suggested simplification
- autoFocus={!!(sourceSearchTerm && !destinationSearchTerm)} + autoFocus={sourceSearchTerm && !destinationSearchTerm}packages/javascript/bh-shared-ui/src/hooks/useKeybindings/useKeybindings.tsx (1)
40-41: Clarify the lowercase fallback logic.The code attempts a lowercase fallback with
bindingsMap[key?.toLowerCase()], butevent.codevalues like"KeyA"or"Digit1"lowercased become"keya"or"digit1", which likely don't match the expected binding keys. This fallback may never match and could be dead code.Consider either removing the fallback or documenting when it's expected to match.
cmd/ui/src/components/KeyboardShortcutsDialog/KeyboardShortcutsDialog.tsx (2)
30-35: Consider validating shortcut map structure.The
parseShortcutEntrieshelper assumes the shortcut map contains exactly one entry and directly accessesentries[0]without validation. This could throw a runtime error if an empty map is passed.🔎 Proposed defensive implementation
const parseShortcutEntries = (shortCutMap: ShortCutsMap) => { const entries = Object.entries(shortCutMap); + if (entries.length === 0) { + return { heading: '', bindings: [] }; + } const [heading, bindings] = entries[0]; return { heading, bindings }; };
41-43: Consider removing unnecessary useCallback wrapper.The
handleClosecallback is a simple passthrough that only callsonClose(). SinceonCloseis already stable (comes from props), wrapping it inuseCallbackprovides no performance benefit and adds unnecessary complexity.🔎 Suggested simplification
- const handleClose = useCallback(() => { - onClose(); - }, [onClose]); - return ( <Dialog open={open} data-testid='keyboard-shortcuts-dialog'> <DialogPortal> - <DialogContent className='h-3/4 grid-rows[50px, 1fr, 50px] max-w-[900px]' onEscapeKeyDown={handleClose}> + <DialogContent className='h-3/4 grid-rows[50px, 1fr, 50px] max-w-[900px]' onEscapeKeyDown={onClose}> <DialogTitle className='text-lg flex justify-center'>Keyboard Shortcuts</DialogTitle> <DialogDescription hidden>Keyboard Shortcuts List</DialogDescription> <hr /> <div className='overflow-auto grid grid-cols-2 gap-3 justify-center'> <div> <ShortcutSection {...parseShortcutEntries(GLOBAL_SHORTCUTS)} /> <ShortcutSection {...parseShortcutEntries(POSTURE_PAGE_SHORTCUTS)} /> </div> <ShortcutSection {...parseShortcutEntries(EXPLORE_SHORTCUTS)} /> </div> <hr /> <DialogActions> - <Button onClick={handleClose} data-testid='keyboard-shortcuts-dialog_button-close'> + <Button onClick={onClose} data-testid='keyboard-shortcuts-dialog_button-close'> Close </Button> </DialogActions>cmd/ui/src/views/Explore/GraphView.tsx (1)
139-174: Consider documenting the keyboard shortcuts behavior.The keyboard shortcuts implement non-trivial UX behaviors (conditional tab switching, complex info panel toggling) that would benefit from inline comments explaining the intended user experience, especially for the KeyI logic.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
cmd/ui/src/App.tsx(4 hunks)cmd/ui/src/components/KeyboardShortcutsDialog/KeyboardShortcutsDialog.tsx(1 hunks)cmd/ui/src/components/KeyboardShortcutsDialog/ShortcutSection.tsx(1 hunks)cmd/ui/src/components/KeyboardShortcutsDialog/index.ts(1 hunks)cmd/ui/src/components/KeyboardShortcutsDialog/shortcuts.ts(1 hunks)cmd/ui/src/components/MainNav/MainNavData.tsx(3 hunks)cmd/ui/src/ducks/global/types.ts(0 hunks)cmd/ui/src/views/Content.tsx(4 hunks)cmd/ui/src/views/Explore/DialogProviders.tsx(1 hunks)cmd/ui/src/views/Explore/GraphView.tsx(3 hunks)packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx(5 hunks)packages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/GraphControls/GraphControls.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.tsx(1 hunks)packages/javascript/bh-shared-ui/src/hooks/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/hooks/useExploreSelectedItem.tsx(3 hunks)packages/javascript/bh-shared-ui/src/hooks/useKeybindings/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/hooks/useKeybindings/useKeybindings.tsx(1 hunks)packages/javascript/bh-shared-ui/src/hooks/useKeyboardShortcutsDialogContext.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/NodeSearch.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/PathfindingSearch.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx(4 hunks)
💤 Files with no reviewable changes (1)
- cmd/ui/src/ducks/global/types.ts
🧰 Additional context used
🧠 Learnings (16)
📚 Learning: 2025-08-28T19:26:03.304Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/views/ZoneManagement/ZoneAnalysisIcon.tsx:26-26
Timestamp: 2025-08-28T19:26:03.304Z
Learning: In packages/javascript/bh-shared-ui/src/hooks/, useZonePathParams is exported through the useZoneParams barrel (useZoneParams/index.ts exports it via wildcard from useZonePathParams.tsx), and usePrivilegeZoneAnalysis is exported through useConfiguration.ts. Both are available via the main hooks barrel import.
Applied to files:
packages/javascript/bh-shared-ui/src/hooks/useKeybindings/index.tspackages/javascript/bh-shared-ui/src/hooks/index.tspackages/javascript/bh-shared-ui/src/components/GraphControls/GraphControls.tsxpackages/javascript/bh-shared-ui/src/hooks/useKeyboardShortcutsDialogContext.tsxpackages/javascript/bh-shared-ui/src/hooks/useKeybindings/useKeybindings.tsxcmd/ui/src/views/Content.tsxcmd/ui/src/App.tsxcmd/ui/src/components/MainNav/MainNavData.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx
📚 Learning: 2025-08-25T20:12:35.629Z
Learnt from: mistahj67
Repo: SpecterOps/BloodHound PR: 1803
File: packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/SummaryCard.tsx:24-24
Timestamp: 2025-08-25T20:12:35.629Z
Learning: The useHighestPrivilegeTagId hook is available through the hooks barrel export in packages/javascript/bh-shared-ui/src/hooks/index.ts via the wildcard export `export * from './useAssetGroupTags'`. The import `import { useHighestPrivilegeTagId } from '../../../hooks'` works correctly and doesn't cause build failures.
Applied to files:
packages/javascript/bh-shared-ui/src/hooks/useKeybindings/index.tspackages/javascript/bh-shared-ui/src/hooks/index.tscmd/ui/src/views/Content.tsxcmd/ui/src/components/MainNav/MainNavData.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx
📚 Learning: 2025-11-06T21:23:30.689Z
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx:0-0
Timestamp: 2025-11-06T21:23:30.689Z
Learning: In ExploreSearchCombobox component (packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx), setting aria-autocomplete={null} on the TextField is intentional to override downshift's default aria-autocomplete attribute, which was flagged by Axe DevTools. This approach satisfies Axe accessibility requirements by removing the conflicting attribute.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/PathfindingSearch.tsxpackages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsxpackages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/NodeSearch.tsxpackages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx
📚 Learning: 2025-11-06T21:35:45.118Z
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx:86-90
Timestamp: 2025-11-06T21:35:45.118Z
Learning: In CypherSearch.tsx (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx), the useLayoutEffect that directly sets aria-label on cypherEditorRef.current.cypherEditor.codemirror.contentDOM is necessary because the aria-label prop passed to the CypherEditor component (neo4j-cypher/react-codemirror) does not properly reach the correct DOM element for accessibility. The direct DOM manipulation is required to satisfy Axe DevTools requirements.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/PathfindingSearch.tsxpackages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsxpackages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.tsxpackages/javascript/bh-shared-ui/src/components/GraphControls/GraphControls.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/NodeSearch.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx
📚 Learning: 2025-08-27T19:48:23.432Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/CommonSearches.tsx:75-85
Timestamp: 2025-08-27T19:48:23.432Z
Learning: In BloodHound's CommonSearches component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/CommonSearches.tsx), calling onPerformCypherSearch('') with an empty string when deselecting a query is intentional behavior, even when auto-run is enabled.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/PathfindingSearch.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/NodeSearch.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx
📚 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/Explore/ExploreSearch/PathfindingSearch.tsxpackages/javascript/bh-shared-ui/src/components/GraphControls/GraphControls.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/NodeSearch.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx
📚 Learning: 2025-11-06T21:35:02.751Z
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/Navigation/AppLink.tsx:30-30
Timestamp: 2025-11-06T21:35:02.751Z
Learning: In AppLink component (packages/javascript/bh-shared-ui/src/components/Navigation/AppLink.tsx), the aria-label="Navigate to ${path}" is added as a fallback for cases where children may be non-string content (icons, JSX elements) rather than readable text.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/PathfindingSearch.tsxpackages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx
📚 Learning: 2025-08-27T19:22:50.905Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/CommonSearches.tsx:71-74
Timestamp: 2025-08-27T19:22:50.905Z
Learning: In BloodHound's CommonSearches component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/CommonSearches.tsx), the useEffect that resets filteredList to queryList when userQueries.data changes is intentional behavior - filters should be cleared when the underlying saved queries data updates.
Applied to files:
packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsxpackages/javascript/bh-shared-ui/src/components/GraphControls/GraphControls.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/NodeSearch.tsxpackages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsxpackages/javascript/bh-shared-ui/src/hooks/useExploreSelectedItem.tsx
📚 Learning: 2025-09-08T19:38:54.755Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 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:
cmd/ui/src/views/Explore/DialogProviders.tsxcmd/ui/src/components/KeyboardShortcutsDialog/KeyboardShortcutsDialog.tsxcmd/ui/src/views/Content.tsxpackages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsxcmd/ui/src/App.tsx
📚 Learning: 2025-09-24T16:36:12.870Z
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 1923
File: packages/javascript/bh-shared-ui/src/components/FileUploadDialog/FileUploadDialog.tsx:55-58
Timestamp: 2025-09-24T16:36:12.870Z
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.
Applied to files:
cmd/ui/src/views/Explore/DialogProviders.tsxcmd/ui/src/components/KeyboardShortcutsDialog/KeyboardShortcutsDialog.tsxcmd/ui/src/views/Content.tsxpackages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.tsxcmd/ui/src/App.tsx
📚 Learning: 2025-08-11T23:51:32.709Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1749
File: packages/javascript/bh-shared-ui/src/hooks/useExploreSelectedItem.tsx:40-46
Timestamp: 2025-08-11T23:51:32.709Z
Learning: The useGraphItem hook in packages/javascript/bh-shared-ui/src/hooks/useGraphItem.tsx already has an enabled field set to !!itemId, which prevents the query from executing when itemId is falsy, providing built-in null-safety.
Applied to files:
packages/javascript/bh-shared-ui/src/components/GraphControls/GraphControls.tsxcmd/ui/src/views/Explore/GraphView.tsxpackages/javascript/bh-shared-ui/src/hooks/useExploreSelectedItem.tsx
📚 Learning: 2025-08-27T19:58:12.996Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/SaveQueryDialog.tsx:89-97
Timestamp: 2025-08-27T19:58:12.996Z
Learning: In BloodHound's SaveQueryDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/SaveQueryDialog.tsx), the useEffect that sets isNew based on selectedQuery existence (rather than id presence) is intentional behavior - the logic for determining whether a query is new vs existing should not be changed to check for id presence.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/NodeSearch.tsxpackages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.tsxcmd/ui/src/views/Explore/GraphView.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsxpackages/javascript/bh-shared-ui/src/hooks/useExploreSelectedItem.tsx
📚 Learning: 2025-08-11T18:25:23.685Z
Learnt from: TheNando
Repo: SpecterOps/BloodHound PR: 1737
File: packages/javascript/bh-shared-ui/src/components/StatusIndicator.tsx:23-35
Timestamp: 2025-08-11T18:25:23.685Z
Learning: In the BloodHound bh-shared-ui package, React is automatically injected into scope by the framework, so explicit React imports are not required when using React.FC or other React types in component files.
Applied to files:
packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsxcmd/ui/src/views/Content.tsxcmd/ui/src/App.tsx
📚 Learning: 2025-07-17T16:28:00.770Z
Learnt from: TheNando
Repo: SpecterOps/BloodHound PR: 1693
File: cmd/ui/src/views/Explore/GraphView.tsx:155-159
Timestamp: 2025-07-17T16:28:00.770Z
Learning: In the BloodHound codebase GraphView.tsx, the context menu toggle logic that switches between null and new coordinates (closing on different item clicks) is intentional, expected behavior as of the current implementation, though this may change in the future.
Applied to files:
cmd/ui/src/views/Explore/GraphView.tsx
📚 Learning: 2025-09-08T19:22:49.284Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx:34-35
Timestamp: 2025-09-08T19:22:49.284Z
Learning: In BloodHound's TagToZoneLabelDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx), importing AssetGroupTag type from 'js-client-library' to type tag shapes is incorrect - this type should not be used for typing tags in this context.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx
📚 Learning: 2025-08-11T23:51:32.709Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1749
File: packages/javascript/bh-shared-ui/src/hooks/useExploreSelectedItem.tsx:40-46
Timestamp: 2025-08-11T23:51:32.709Z
Learning: The version of react-query used in this codebase doesn't have typeguard support for itemId when used in the enabled field, so type assertions may still be necessary even with enabled checks in place.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx
🧬 Code graph analysis (10)
packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/useTheme.tsx (1)
useTheme(91-158)
cmd/ui/src/views/Explore/DialogProviders.tsx (2)
packages/javascript/bh-shared-ui/src/hooks/useFileUploadDialogContext.tsx (1)
FileUploadDialogProvider(26-35)packages/javascript/bh-shared-ui/src/hooks/useKeyboardShortcutsDialogContext.tsx (1)
KeyboardShortcutsDialogProvider(26-35)
packages/javascript/bh-shared-ui/src/components/GraphControls/GraphControls.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/useKeybindings/useKeybindings.tsx (1)
useKeybindings(27-60)
cmd/ui/src/components/KeyboardShortcutsDialog/KeyboardShortcutsDialog.tsx (1)
cmd/ui/src/components/KeyboardShortcutsDialog/shortcuts.ts (4)
ShortCutsMap(16-16)GLOBAL_SHORTCUTS(32-39)POSTURE_PAGE_SHORTCUTS(41-43)EXPLORE_SHORTCUTS(18-30)
packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/useKeybindings/useKeybindings.tsx (1)
useKeybindings(27-60)
cmd/ui/src/views/Content.tsx (2)
packages/javascript/bh-shared-ui/src/hooks/useKeyboardShortcutsDialogContext.tsx (1)
useKeyboardShortcutsDialogContext(37-37)packages/javascript/bh-shared-ui/src/hooks/useKeybindings/useKeybindings.tsx (1)
useKeybindings(27-60)
cmd/ui/src/views/Explore/GraphView.tsx (4)
packages/javascript/bh-shared-ui/src/hooks/useExploreSelectedItem.tsx (1)
useExploreSelectedItem(22-65)packages/javascript/bh-shared-ui/src/hooks/useExploreParams/useExploreParams.tsx (1)
useExploreParams(77-112)packages/javascript/bh-shared-ui/src/hooks/useKeybindings/useKeybindings.tsx (1)
useKeybindings(27-60)cmd/ui/src/ducks/global/actions.ts (1)
setIsExploreTableSelected(74-79)
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/useKeybindings/useKeybindings.tsx (1)
useKeybindings(27-60)
cmd/ui/src/App.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/useKeybindings/useKeybindings.tsx (1)
useKeybindings(27-60)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/useKeybindings/useKeybindings.tsx (1)
useKeybindings(27-60)
🔇 Additional comments (20)
packages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.tsx (2)
20-20: LGTM: Import addition is correct.The
useEffectimport is necessary for the keyboard event listener cleanup logic added below.
120-137: The current Escape key handler implementation is appropriate for this use case. TheuseKeybindingshook is specifically designed for Alt+key navigation shortcuts and only listens for Alt+key combinations—it does not intercept plain Escape key events. Since ExploreTable needs to close on a plain Escape keypress, using document-level event listeners for this specific purpose is the correct approach. No conflicts exist with the keyboard shortcuts system, and the implementation follows standard patterns for dismissible components.Likely an incorrect or invalid review comment.
packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.tsx (1)
144-146: Clarify the reason for switching from autoFocus prop to ref callback.The change from using the
autoFocusprop to aninputRefcallback achieves similar behavior but alters the timing and approach. The ref callback executes when the input is mounted or updated, whileautoFocusis a more declarative, standard React pattern.Could you clarify the motivation for this change? Is it to avoid focus conflicts with the keyboard shortcuts system, manage focus timing with dialogs, or address a specific accessibility concern?
packages/javascript/bh-shared-ui/src/hooks/useKeybindings/index.ts (1)
1-17: LGTM!Standard barrel export with correct license header. The re-export pattern aligns with the project's hook organization structure.
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/PathfindingSearch.tsx (1)
61-61: LGTM!The conditional autofocus on the Start Node input when empty provides good UX for progressive form filling.
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/NodeSearch.tsx (1)
32-32: LGTM!Adding autofocus to the node search input improves keyboard navigation and provides a better user experience when the search view opens.
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx (2)
243-246: LGTM!The keyboard shortcuts are well-integrated:
- KeyR triggers the search execution
- KeyS intelligently toggles between opening and closing the save dialog based on context
The conditional logic for KeyS correctly handles both states of the dialog.
316-316: Good simplification of the onClick handler.Changing from an inline arrow function to a direct reference improves readability and avoids unnecessary function recreation on each render.
cmd/ui/src/components/MainNav/MainNavData.tsx (1)
93-93: LGTM!The keyboard shortcut for dark mode toggle is properly gated by authentication state, preventing unauthorized access. The integration follows the established pattern for useKeybindings.
Also applies to: 109-113
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx (1)
39-39: LGTM!The keyboard shortcut implementation correctly:
- Creates a ref for the input element
- Forwards the ref through downshift's
getInputProps- Includes a null check before calling
focus()- Enables users to quickly access search with the Slash key
The pattern aligns well with the broader keyboard shortcuts infrastructure introduced in this PR.
Also applies to: 98-104, 116-116
cmd/ui/src/components/KeyboardShortcutsDialog/index.ts (1)
16-18: LGTM!Standard barrel export pattern with proper license header.
packages/javascript/bh-shared-ui/src/hooks/index.ts (1)
42-43: LGTM!Barrel exports follow the established pattern in this file.
cmd/ui/src/components/KeyboardShortcutsDialog/ShortcutSection.tsx (1)
16-33: LGTM! Clean presentational component.The component correctly renders keyboard shortcuts with proper key attributes for the mapped bindings array.
cmd/ui/src/views/Explore/DialogProviders.tsx (1)
19-23: LGTM!Clean provider composition following React best practices.
packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx (1)
73-81: LGTM! Clean ref wiring and prop handling.The downshiftInputProps wrapper properly consolidates Downshift's input props with custom handlers, and the dual ref wiring (lines 103-106) correctly passes the ref to both Downshift and the local ref for external access.
cmd/ui/src/App.tsx (1)
94-98: Consider adding authentication guard to external URL keybinding.The
KeyDkeybinding opens an external URL without checkingfullyAuthenticated, while the keybindings inContent.tsx(KeyH, KeyU) explicitly guard withisFullyAuthenticated. Although this code runs after authentication initialization (line 101 check), it will execute for any authenticated session, including partially authenticated states.Consider whether this behavior is intentional or if it should follow the same pattern as Content.tsx:
KeyD: () => { if (fullyAuthenticated) { window.open('https://bloodhound.specterops.io/home', '_blank'); } },Please verify that opening the documentation link should be available to partially authenticated users, or if it should require full authentication like other shortcuts.
packages/javascript/bh-shared-ui/src/hooks/useKeyboardShortcutsDialogContext.tsx (1)
1-37: LGTM! Clean context implementation.The context follows standard React patterns with proper TypeScript typing, sensible defaults, and a clean provider/consumer API.
cmd/ui/src/views/Explore/GraphView.tsx (1)
160-173: Verify edge case handling in KeyI toggle logic.The KeyI shortcut implements "Toggle Node Info Panel" with conditional logic that:
- Clears selection if an item is currently selected
- Selects the only node if exactly one exists in results
- Falls back to
previousSelectedItemotherwiseHowever, when
previousSelectedItemis falsy (empty string or undefined), line 171 will callsetSelectedItem(''), which may not be distinguishable from case 1 (explicitly clearing). This could lead to ambiguous state.Consider whether the fallback should distinguish between "no previous selection" and "explicitly cleared":
KeyI: () => { const entries = Object.entries(graphQuery?.data || {}); const onlyOneNodeInResults = entries.length === 1; if (selectedItem) { setSelectedItem(''); } else if (onlyOneNodeInResults) { const onlyNodeKey = entries[0][0]; setSelectedItem(onlyNodeKey); } else if (previousSelectedItem) { setSelectedItem(previousSelectedItem); } // else: no-op if no previous selection exists },This prevents calling
setSelectedItem('')when there's nothing to restore.cmd/ui/src/views/Content.tsx (2)
75-82: LGTM! Proper authentication guards on keyboard shortcuts.The keybindings for KeyH and KeyU correctly guard access with
isFullyAuthenticated, ensuring shortcuts only function when the user has full access. This aligns with the conditional rendering of dialogs at lines 122-133.
123-132: LGTM! Clean dialog integration with proper authentication guard.The KeyboardShortcutsDialog is correctly integrated alongside FileUploadDialog with appropriate authentication gating and state management through the context hook.
| return ( | ||
| <Dialog open={open} data-testid='keyboard-shortcuts-dialog'> | ||
| <DialogPortal> | ||
| <DialogContent className='h-3/4 grid-rows[50px, 1fr, 50px] max-w-[900px]' onEscapeKeyDown={handleClose}> |
Contributor
There was a problem hiding this comment.
Fix invalid Tailwind CSS syntax.
The className grid-rows[50px, 1fr, 50px] uses invalid Tailwind syntax. Tailwind's arbitrary values for grid-rows should use underscores instead of commas, and the correct property name is grid-template-rows.
🔎 Proposed fix
- <DialogContent className='h-3/4 grid-rows[50px, 1fr, 50px] max-w-[900px]' onEscapeKeyDown={handleClose}>
+ <DialogContent className='h-3/4 grid-template-rows-[50px_1fr_50px] max-w-[900px]' onEscapeKeyDown={handleClose}>Alternatively, if this grid structure is intended for the content wrapper, ensure the parent has display: grid:
- <DialogContent className='h-3/4 grid-rows[50px, 1fr, 50px] max-w-[900px]' onEscapeKeyDown={handleClose}>
+ <DialogContent className='h-3/4 grid grid-rows-[50px_1fr_50px] max-w-[900px]' onEscapeKeyDown={handleClose}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <DialogContent className='h-3/4 grid-rows[50px, 1fr, 50px] max-w-[900px]' onEscapeKeyDown={handleClose}> | |
| <DialogContent className='h-3/4 grid grid-rows-[50px_1fr_50px] max-w-[900px]' onEscapeKeyDown={handleClose}> |
🤖 Prompt for AI Agents
In cmd/ui/src/components/KeyboardShortcutsDialog/KeyboardShortcutsDialog.tsx
around line 48, the Tailwind class `grid-rows[50px, 1fr, 50px]` is invalid;
replace it with the correct arbitrary-value syntax using underscores and
brackets (e.g. `grid-rows-[50px_1fr_50px]`) and ensure the element (or its
parent) has `grid` in its class list so the row template is applied (add `grid`
if missing); keep the rest of the className intact.
Comment on lines
+18
to
+43
| export const EXPLORE_SHORTCUTS = { | ||
| 'Explore Page': [ | ||
| ['S', 'Jump to Node Search'], | ||
| ['P', 'Jump to Pathfinding'], | ||
| ['C', 'Focus Cypher Query Editor'], | ||
| ['Shift + S', 'Save Current Query'], | ||
| ['R', 'Run Current Cypher Query'], | ||
| ['/', 'Search Current Nodes'], | ||
| ['T', 'Toggle Table View'], | ||
| ['I', 'Toggle Node Info Panel'], | ||
| ['Shift + G', 'Reset Graph View'], | ||
| ], | ||
| }; | ||
|
|
||
| export const GLOBAL_SHORTCUTS = { | ||
| Global: [ | ||
| ['<integer>', 'Navigate sidebar pages'], | ||
| ['H', 'Launch keyboard shortcuts list'], | ||
| ['U', 'Launch File Upload dialog'], | ||
| ['M', 'Toggle Dark Mode'], | ||
| ], | ||
| }; | ||
|
|
||
| export const POSTURE_PAGE_SHORTCUTS = { | ||
| 'Posture Page': [['F', 'Filter Table Data']], | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for keybinding implementations across the codebase
echo "=== Searching for KeyR (Run Cypher Query) ==="
rg -n "KeyR:" --type=tsx --type=ts
echo "=== Searching for Slash key binding (Search Nodes) ==="
rg -n "Slash:" --type=tsx --type=ts
echo "=== Searching for Shift + S (Save Query) ==="
rg -n "shift.*KeyS" --type=tsx --type=ts -i
echo "=== Searching for Shift + G (Reset Graph) ==="
rg -n "shift.*KeyG" --type=tsx --type=ts -i
echo "=== Searching for Attack Paths keybindings (E, K, J, R) ==="
rg -n "KeyE:|KeyK:|KeyJ:" --type=tsx --type=ts -A2 -B2
echo "=== Searching for Posture page keybindings (F, Z) ==="
rg -n "KeyF:|KeyZ:" --type=tsx --type=ts -A2 -B2
echo "=== Searching for keyboard event handlers in the codebase ==="
rg -n "onKeyDown|keyCode|key ===" --type=tsx --type=ts | head -30Repository: SpecterOps/BloodHound
Length of output: 645
🏁 Script executed:
#!/bin/bash
# Search for keybinding implementations - broader search
echo "=== Finding files with keyboard event handlers ==="
find . -name "*.ts" -o -name "*.js" | xargs grep -l "onKeyDown\|keyCode\|key ===" | head -20
echo -e "\n=== Searching for KeyR pattern ==="
grep -r "KeyR" . --include="*.ts" --include="*.js" -n | head -10
echo -e "\n=== Searching for Slash key binding ==="
grep -r "Slash" . --include="*.ts" --include="*.js" -n | head -10
echo -e "\n=== Searching for keyboard handlers in Explore/Query context ==="
grep -r "Save.*Query\|Run.*Cypher\|Search.*Nodes\|Reset.*Graph" . --include="*.ts" --include="*.js" -n | head -20
echo -e "\n=== Searching for Attack Paths shortcuts ==="
grep -r "Attack.*Path\|Option.*Shift.*R" . --include="*.ts" --include="*.js" -n | head -10
echo -e "\n=== Searching for shortcuts file itself and related files ==="
find . -name "*shortcut*" -o -name "*keyboard*" | grep -E "\.(ts|js|tsx|jsx)$"Repository: SpecterOps/BloodHound
Length of output: 5332
🏁 Script executed:
#!/bin/bash
echo "=== Examining global reducer for keyboard handlers ==="
cat -n ./cmd/ui/src/ducks/global/reducer.ts | head -100
echo -e "\n=== Searching for keyboard-related actions/handlers ==="
grep -r "keyboard\|shortcut\|keybinding" . --include="*.ts" --include="*.js" -i | grep -v node_modules | head -20
echo -e "\n=== Looking for key event handling in components ==="
find . -path ./node_modules -prune -o -name "*.ts" -o -name "*.js" | xargs grep -l "event.key\|event.code" | head -10
echo -e "\n=== Checking the shortcuts file more carefully ==="
cat -n ./cmd/ui/src/components/KeyboardShortcutsDialog/shortcuts.tsRepository: SpecterOps/BloodHound
Length of output: 7785
🏁 Script executed:
#!/bin/bash
echo "=== Examining useKeybindings hook ==="
find . -path ./node_modules -prune -o -name "*useKeybindings*" -type f -print | head -20
echo -e "\n=== Reading useKeybindings implementation ==="
find ./packages/javascript/bh-shared-ui/src/hooks -name "useKeybindings*" -type f -exec cat -n {} \;
echo -e "\n=== Searching for Attack Paths and Posture page keybinding implementations ==="
grep -r "attackpath\|attack.path" . --include="*.ts" --include="*.tsx" -i | grep -E "key|shortcut|keyboard" | head -20Repository: SpecterOps/BloodHound
Length of output: 2810
🏁 Script executed:
#!/bin/bash
echo "=== Searching for useKeybindings usage and bindings definitions ==="
grep -r "useKeybindings" . --include="*.ts" --include="*.tsx" -A10 | head -100
echo -e "\n=== Searching for keyboard shortcut implementations on different pages ==="
grep -r "KeyBindings\|shift.*KeyBindings" . --include="*.ts" --include="*.tsx" | head -30
echo -e "\n=== Looking for Explore, Posture, and Attack Paths page components ==="
find . -name "*Explore*.tsx" -o -name "*Posture*.tsx" -o -name "*AttackPath*.tsx" | grep -v node_modules | head -20Repository: SpecterOps/BloodHound
Length of output: 9554
🏁 Script executed:
#!/bin/bash
echo "=== Getting full GraphView.tsx keybindings ==="
grep -n "useKeybindings" ./cmd/ui/src/views/Explore/GraphView.tsx -A50 | head -100
echo -e "\n=== Searching for all keybindings across the Explore views ==="
find ./cmd/ui/src/views -name "*.tsx" -exec grep -l "useKeybindings" {} \;
echo -e "\n=== Checking for Posture page implementation ==="
find ./cmd/ui/src/views -name "*Posture*" -o -name "*posture*" | head -20
echo -e "\n=== Searching for Attack Paths page ==="
find ./cmd/ui/src/views -name "*Attack*" -o -name "*attack*" | head -20
echo -e "\n=== Checking for Save Query, Run Query keybindings ==="
grep -r "Save.*Query\|Run.*Query\|Cypher" ./cmd/ui/src --include="*.tsx" | grep -i "key\|shift" | head -20Repository: SpecterOps/BloodHound
Length of output: 4812
🏁 Script executed:
#!/bin/bash
echo "=== Finding all useKeybindings calls and their implementations ==="
grep -r "useKeybindings({" . --include="*.tsx" --include="*.ts" -A30 | grep -E "(useKeybindings|Key[A-Z]|shift:)" | head -100
echo -e "\n=== Checking for Shift keybindings (shift: { ... }) ==="
grep -r "shift:" . --include="*.tsx" --include="*.ts" -A20 | head -80
echo -e "\n=== Searching for GraphControls keybindings ==="
grep -n "useKeybindings" ./packages/javascript/bh-shared-ui/src/components/GraphControls/GraphControls.tsx -A30
echo -e "\n=== Checking ExploreSearch for keybindings ==="
grep -n "useKeybindings" ./cmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.tsx -A30Repository: SpecterOps/BloodHound
Length of output: 4033
🏁 Script executed:
#!/bin/bash
echo "=== Reading CypherSearch.tsx keybindings ==="
cat -n ./packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx | head -150
echo -e "\n=== Searching for 'S' keybinding (Jump to Node Search) ==="
grep -r "KeyS" . --include="*.tsx" --include="*.ts" -B5 -A5 | head -40
echo -e "\n=== Searching for integer/number keybindings for sidebar navigation ==="
grep -r "integer\|<[0-9]>\|Navigate.*sidebar" . --include="*.tsx" --include="*.ts" | head -20
echo -e "\n=== Searching for Filter Table Data (F key) implementation ==="
grep -r "Filter.*Table\|KeyF" . --include="*.tsx" --include="*.ts" | head -20Repository: SpecterOps/BloodHound
Length of output: 9551
🏁 Script executed:
#!/bin/bash
echo "=== Re-examining useKeybindings hook implementation detail ==="
cat -n ./packages/javascript/bh-shared-ui/src/hooks/useKeybindings/useKeybindings.tsx
echo -e "\n=== Checking for shift keybindings pattern ==="
grep -r "shift:" . --include="*.tsx" --include="*.ts" -B2 -A10 | head -50
echo -e "\n=== Looking for Attack Paths page component files ==="
find . -path ./node_modules -prune -o -name "*[Aa]ttack*" -type f -print | grep -E "\.(tsx|ts)$"
echo -e "\n=== Checking if there's a PR description or commit message ==="
git log --oneline -1
echo -e "\n=== Searching for all keyboard event handlers more broadly ==="
grep -r "altKey\|event.key\|event.code" . --include="*.tsx" --include="*.ts" | grep -v node_modules | head -20Repository: SpecterOps/BloodHound
Length of output: 3478
🏁 Script executed:
#!/bin/bash
echo "=== Checking if there's any documentation about Alt modifier requirement ==="
grep -r "Alt\|altKey\|Option\|Control" ./cmd/ui/src/components/KeyboardShortcutsDialog/ --include="*.ts" --include="*.tsx"
echo -e "\n=== Checking for any test files that show how keybindings work ==="
find . -name "*keybinding*test*" -o -name "*keyboard*test*" | grep -E "\.(tsx|ts)$"
echo -e "\n=== Searching for usages of Shift keybindings ==="
grep -r "KeyR\|KeyS\|KeyG" . --include="*.tsx" --include="*.ts" -B5 -A5 | grep -i "shift" | head -30
echo -e "\n=== Checking git history for recent changes to shortcuts ==="
git log --oneline ./cmd/ui/src/components/KeyboardShortcutsDialog/shortcuts.ts | head -10
echo -e "\n=== Looking for any routes or pages for Attack Paths ==="
grep -r "attack.*path\|attackpath" . --include="*.tsx" --include="*.ts" -i | grep -E "route|page|view" | head -20Repository: SpecterOps/BloodHound
Length of output: 3540
🏁 Script executed:
#!/bin/bash
echo "=== Reading ShortcutSection.tsx to understand display ==="
cat -n ./cmd/ui/src/components/KeyboardShortcutsDialog/ShortcutSection.tsx
echo -e "\n=== Checking if Shift keybindings are supported in useKeybindings ==="
grep -r "event.shiftKey" ./packages/javascript/bh-shared-ui/src/hooks/useKeybindings/ -B5 -A5
echo -e "\n=== Verifying all documented Explore shortcuts have implementations ==="
echo "Documented in shortcuts.ts:"
cat ./cmd/ui/src/components/KeyboardShortcutsDialog/shortcuts.ts | grep -o "\['[^']*'" | head -20
echo ""
echo "Implemented (found in code):"
echo "KeyC - Focus Cypher Query Editor"
echo "KeyN - Jump to Node Search"
echo "KeyP - Jump to Pathfinding"
echo "KeyT - Toggle Table View"
echo "KeyI - Toggle Node Info Panel"
echo "KeyR - Run Current Cypher Query"
echo "KeyS - Save Current Query"
echo "Slash - Search Current Nodes"
echo "KeyG - Reset Graph View"Repository: SpecterOps/BloodHound
Length of output: 4195
Verify completeness and accuracy of documented shortcuts.
The documentation contains several critical discrepancies with actual implementations:
Key mapping errors:
- Line 20: Documents 'S' for "Jump to Node Search" but implementation uses KeyN
- Line 23-24: Documents "Shift + S" for "Save Current Query" but implementation is KeyS (Alt+S, no shift modifier)
- Line 28: Documents "Shift + G" but no Alt+Shift+G implementation exists; only KeyG (Alt+G) for Reset Graph View is implemented
Missing implementations not documented:
- Attack Paths Page shortcuts (mentioned in PR description)
- Posture Page: Option/Control + E (Environment Selector) and Z (Zone Selector)
- F key for "Filter Table Data" (listed but not implemented)
<integer>sidebar navigation keys
Technical issue:
The useKeybindings hook requires Alt/Option key to trigger shortcuts. The documentation shows "Shift + S" and "Shift + G" in the shortcuts file, but the hook doesn't support Shift+Alt combinations as currently written—only Alt+key combinations work.
Verified implementations:
- KeyC, KeyN, KeyP (Explore page navigation)
- KeyT, KeyI (Table and Info Panel toggles)
- KeyR, KeyS (Run and Save queries)
- Slash (Search Current Nodes)
- KeyH, KeyU, KeyM (Global shortcuts)
- KeyD (Documentation link, undocumented)
🤖 Prompt for AI Agents
In cmd/ui/src/components/KeyboardShortcutsDialog/shortcuts.ts around lines 18 to
43, the documented shortcut entries are inconsistent with the actual
useKeybindings implementation (which requires the Alt/Option modifier) and miss
several implemented shortcuts; update the constants so labels and modifiers
match real key events (use "Alt + <key>" rather than "Shift + ..." for
Alt-triggered actions), correct key names (e.g., document KeyN for Node Search,
KeyC/KeyP for Explore navigation, KeyS/Alt+S for Save/Run as implemented),
replace "Shift + G" with "Alt + G" (Reset Graph View) and remove unsupported
Shift+Alt combos, add missing entries for Attack Paths page shortcuts, Posture
Page Option/Control + E and Z, the F key for filtering, the numeric sidebar
navigation ("<integer>"/"Alt + <n>"), and include the undocumented KeyD
documentation shortcut; ensure all labels explicitly state the Alt/Option
modifier to match useKeybindings.
|
|
||
| export const EXPLORE_SHORTCUTS = { | ||
| 'Explore Page': [ | ||
| ['S', 'Jump to Node Search'], |
Contributor
There was a problem hiding this comment.
Fix inconsistency between shortcut description and implementation.
Line 20 documents 'S' as "Jump to Node Search", but the actual implementation in GraphView.tsx (line 147) uses KeyN to switch to the node tab. Additionally, the PR description states the shortcut should be "Option/Control + N".
🔎 Proposed fix
export const EXPLORE_SHORTCUTS = {
'Explore Page': [
- ['S', 'Jump to Node Search'],
+ ['N', 'Jump to Node Search'],
['P', 'Jump to Pathfinding'],
['C', 'Focus Cypher Query Editor'],
['Shift + S', 'Save Current Query'],
['R', 'Run Current Cypher Query'],
['/', 'Search Current Nodes'],
['T', 'Toggle Table View'],
['I', 'Toggle Node Info Panel'],
['Shift + G', 'Reset Graph View'],
],
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ['S', 'Jump to Node Search'], | |
| export const EXPLORE_SHORTCUTS = { | |
| 'Explore Page': [ | |
| ['N', 'Jump to Node Search'], | |
| ['P', 'Jump to Pathfinding'], | |
| ['C', 'Focus Cypher Query Editor'], | |
| ['Shift + S', 'Save Current Query'], | |
| ['R', 'Run Current Cypher Query'], | |
| ['/', 'Search Current Nodes'], | |
| ['T', 'Toggle Table View'], | |
| ['I', 'Toggle Node Info Panel'], | |
| ['Shift + G', 'Reset Graph View'], | |
| ], | |
| }; |
🤖 Prompt for AI Agents
In cmd/ui/src/components/KeyboardShortcutsDialog/shortcuts.ts around line 20,
the shortcut entry currently lists 'S' for "Jump to Node Search" but the
implementation uses KeyN and the PR specifies Option/Control + N; replace the
'S' label with the correct display string "Option/Control + N" (or the project's
established format for Alt/Ctrl combos) so the documentation matches
GraphView.tsx and the PR description, and remove any duplicate/conflicting 'S'
entry if present.
| }; | ||
| }, [handleKeydown]); | ||
|
|
||
| onClose; |
Contributor
There was a problem hiding this comment.
Remove dead code.
This standalone statement references onClose but has no side effects. It appears to be leftover code that should be removed.
🔎 Proposed fix
- onClose;
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| onClose; |
🤖 Prompt for AI Agents
In packages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.tsx
around line 139, there is a standalone dead statement "onClose;" with no side
effects; remove this line from the file. After deleting it, ensure there are no
missing imports or usages (confirm onClose is still declared/used appropriately
elsewhere) and run TypeScript/ESLint and the unit/build to verify no
regressions.
Comment on lines
+71
to
+76
| useKeybindings({ | ||
| Slash: () => { | ||
| setIsCurrentSearchOpen(!isCurrentSearchOpen); | ||
| }, | ||
| KeyG: onReset, | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use functional state update to avoid stale closure.
The Slash handler reads isCurrentSearchOpen directly, which may capture a stale value in the closure if multiple keypresses occur before React flushes state updates.
🔎 Recommended fix using functional update
useKeybindings({
Slash: () => {
- setIsCurrentSearchOpen(!isCurrentSearchOpen);
+ setIsCurrentSearchOpen(prev => !prev);
},
KeyG: onReset,
});🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/components/GraphControls/GraphControls.tsx
around lines 71 to 76, the Slash key handler closes over isCurrentSearchOpen
causing potential stale state; change the setIsCurrentSearchOpen call to use the
functional updater form (e.g., setIsCurrentSearchOpen(prev => !prev)) so the
toggle always uses the latest state from React rather than a possibly stale
closure.
Comment on lines
+195
to
+206
| const keybindings = useMemo( | ||
| () => | ||
| [...mainNavData.primaryList, ...mainNavData.secondaryList] | ||
| .filter((navItem) => !!navItem.route) | ||
| .reduce((acc, curr, index) => { | ||
| return { | ||
| ...acc, | ||
| [`Digit${index + 1}`]: () => navigate(curr.route!), | ||
| }; | ||
| }, {}), | ||
| [mainNavData, navigate] | ||
| ); |
Contributor
There was a problem hiding this comment.
Limit keybindings to Digit1-Digit9.
The code generates keybindings for all navigation items using Digit${index + 1}, but valid keyboard codes only include Digit0 through Digit9. If there are more than 9 items with routes, this creates invalid keybindings like Digit10, Digit11, etc., which can never be triggered.
🔎 Proposed fix to limit to first 9 items
const keybindings = useMemo(
() =>
[...mainNavData.primaryList, ...mainNavData.secondaryList]
.filter((navItem) => !!navItem.route)
+ .slice(0, 9)
.reduce((acc, curr, index) => {
return {
...acc,
[`Digit${index + 1}`]: () => navigate(curr.route!),
};
}, {}),
[mainNavData, navigate]
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const keybindings = useMemo( | |
| () => | |
| [...mainNavData.primaryList, ...mainNavData.secondaryList] | |
| .filter((navItem) => !!navItem.route) | |
| .reduce((acc, curr, index) => { | |
| return { | |
| ...acc, | |
| [`Digit${index + 1}`]: () => navigate(curr.route!), | |
| }; | |
| }, {}), | |
| [mainNavData, navigate] | |
| ); | |
| const keybindings = useMemo( | |
| () => | |
| [...mainNavData.primaryList, ...mainNavData.secondaryList] | |
| .filter((navItem) => !!navItem.route) | |
| .slice(0, 9) | |
| .reduce((acc, curr, index) => { | |
| return { | |
| ...acc, | |
| [`Digit${index + 1}`]: () => navigate(curr.route!), | |
| }; | |
| }, {}), | |
| [mainNavData, navigate] | |
| ); |
🤖 Prompt for AI Agents
In packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx around
lines 195 to 206, the keybinding generation uses `Digit${index + 1}` for every
nav item which produces invalid codes like `Digit10+`; limit keybindings to the
first nine routed items by taking only the first 9 items after filtering (e.g.,
.filter(...).slice(0,9)) or by skipping indexes >=9 in the reducer, and map
indexes 0..8 to `Digit1`..`Digit9` so only valid keyboard codes are produced.
| import { useGraphItem } from './useGraphItem'; | ||
|
|
||
| export const useExploreSelectedItem = () => { | ||
| const [previousSelectedItem, setPreviousSelectedItem] = useState<string | null>(); |
Contributor
There was a problem hiding this comment.
Type inconsistency: state initialization doesn't match type annotation.
The state is typed as string | null but initialized without a value, making it undefined initially. This creates a mismatch between the type annotation and runtime behavior.
🔎 Proposed fix
- const [previousSelectedItem, setPreviousSelectedItem] = useState<string | null>();
+ const [previousSelectedItem, setPreviousSelectedItem] = useState<string | null>(null);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const [previousSelectedItem, setPreviousSelectedItem] = useState<string | null>(); | |
| const [previousSelectedItem, setPreviousSelectedItem] = useState<string | null>(null); |
🤖 Prompt for AI Agents
In packages/javascript/bh-shared-ui/src/hooks/useExploreSelectedItem.tsx around
line 23, the state is declared as string | null but left uninitialized so it
starts as undefined; either initialize the state to null to match the annotated
type, or change the type annotation to include undefined (string | null |
undefined) so the runtime initial value and the type are consistent—apply the
chosen change to the useState call and update any downstream consumers if their
logic assumes non-undefined values.
Comment on lines
+29
to
+51
| const handleKeyDown = useCallback( | ||
| (event: KeyboardEvent) => { | ||
| if (event.altKey && !event.metaKey) { | ||
| event.preventDefault(); | ||
|
|
||
| if (event.shiftKey && !bindings.shift) { | ||
| return; | ||
| } | ||
|
|
||
| const bindingsMap: KeyBindingsWithShift = event.shiftKey && bindings.shift ? bindings.shift : bindings; | ||
|
|
||
| const key = event.code; | ||
| const func = bindingsMap[key] || bindingsMap[key?.toLowerCase()]; | ||
|
|
||
| if (typeof func === 'function') { | ||
| func({ | ||
| navigate, | ||
| }); | ||
| } | ||
| } | ||
| }, | ||
| [bindings, navigate] | ||
| ); |
Contributor
There was a problem hiding this comment.
Critical: preventDefault called unconditionally, breaking browser shortcuts.
Line 32 calls event.preventDefault() for ALL Alt+key combinations, even when no matching binding exists. This prevents default browser shortcuts like Alt+D (address bar), Alt+Left (back), Alt+F (file menu), etc. The preventDefault should only be called if a matching binding is found and executed.
🔎 Proposed fix
const handleKeyDown = useCallback(
(event: KeyboardEvent) => {
if (event.altKey && !event.metaKey) {
- event.preventDefault();
if (event.shiftKey && !bindings.shift) {
return;
}
const bindingsMap: KeyBindingsWithShift = event.shiftKey && bindings.shift ? bindings.shift : bindings;
const key = event.code;
const func = bindingsMap[key] || bindingsMap[key?.toLowerCase()];
if (typeof func === 'function') {
+ event.preventDefault();
func({
navigate,
});
}
}
},
[bindings, navigate]
);🤖 Prompt for AI Agents
In packages/javascript/bh-shared-ui/src/hooks/useKeybindings/useKeybindings.tsx
around lines 29 to 51, the handler currently calls event.preventDefault() for
every Alt+key which blocks native browser shortcuts; change the flow so you
first resolve the correct bindings map (considering shift), lookup the function
for event.code (and lowercase fallback), and only call event.preventDefault()
and invoke the function if a matching function exists; leave all other Alt+key
events untouched so browser defaults continue to work.
Major: Missing input element check causes shortcuts to fire while typing.
The hook captures keyboard events globally without checking if the user is typing in an input field, textarea, or contentEditable element. This can interrupt normal text entry and cause unexpected navigation or actions.
🔎 Proposed fix to ignore shortcuts when typing
const handleKeyDown = useCallback(
(event: KeyboardEvent) => {
+ // Ignore shortcuts when user is typing in input fields
+ const target = event.target as HTMLElement;
+ if (
+ target.tagName === 'INPUT' ||
+ target.tagName === 'TEXTAREA' ||
+ target.isContentEditable
+ ) {
+ return;
+ }
+
if (event.altKey && !event.metaKey) {
event.preventDefault();
if (event.shiftKey && !bindings.shift) {
return;
}
const bindingsMap: KeyBindingsWithShift = event.shiftKey && bindings.shift ? bindings.shift : bindings;
const key = event.code;
const func = bindingsMap[key] || bindingsMap[key?.toLowerCase()];
if (typeof func === 'function') {
func({
navigate,
});
}
}
},
[bindings, navigate]
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleKeyDown = useCallback( | |
| (event: KeyboardEvent) => { | |
| if (event.altKey && !event.metaKey) { | |
| event.preventDefault(); | |
| if (event.shiftKey && !bindings.shift) { | |
| return; | |
| } | |
| const bindingsMap: KeyBindingsWithShift = event.shiftKey && bindings.shift ? bindings.shift : bindings; | |
| const key = event.code; | |
| const func = bindingsMap[key] || bindingsMap[key?.toLowerCase()]; | |
| if (typeof func === 'function') { | |
| func({ | |
| navigate, | |
| }); | |
| } | |
| } | |
| }, | |
| [bindings, navigate] | |
| ); | |
| const handleKeyDown = useCallback( | |
| (event: KeyboardEvent) => { | |
| // Ignore shortcuts when user is typing in input fields | |
| const target = event.target as HTMLElement; | |
| if ( | |
| target.tagName === 'INPUT' || | |
| target.tagName === 'TEXTAREA' || | |
| target.isContentEditable | |
| ) { | |
| return; | |
| } | |
| if (event.altKey && !event.metaKey) { | |
| event.preventDefault(); | |
| if (event.shiftKey && !bindings.shift) { | |
| return; | |
| } | |
| const bindingsMap: KeyBindingsWithShift = event.shiftKey && bindings.shift ? bindings.shift : bindings; | |
| const key = event.code; | |
| const func = bindingsMap[key] || bindingsMap[key?.toLowerCase()]; | |
| if (typeof func === 'function') { | |
| func({ | |
| navigate, | |
| }); | |
| } | |
| } | |
| }, | |
| [bindings, navigate] | |
| ); |
🤖 Prompt for AI Agents
In packages/javascript/bh-shared-ui/src/hooks/useKeybindings/useKeybindings.tsx
around lines 29 to 51, the keydown handler doesn't ignore events originating
from text inputs or contentEditable elements, causing shortcuts to trigger while
the user is typing; add an early return that checks the event.target (cast to
HTMLElement) and skips handling if it's an input, textarea, select, or any
element that is contentEditable (or is inside a contentEditable), so the handler
only runs for non-editable contexts.
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.
Description
Screen.Recording.2025-12-17.at.4.19.36.PM.mov
Describe your changes in detail
Motivation and Context
Resolves <TICKET_OR_ISSUE_NUMBER>
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
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.