feat: Far reaching accessibility changes BED-5690 #2010
+1,603
−200
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
Contributor
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Add the WalkthroughWidespread accessibility and keyboard-interaction updates across UI components, addition of a helper Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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
requested review from
elikmiller and
superlinkx
as code owners
![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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx (1)
95-111: Missing accessibility attributes on clickable ListItem.The clickable
ListItemlacks essential accessibility attributes required for keyboard navigation and screen reader support. According to the PR objectives, "keyDown listeners, tabIndex, and role='button'" should be added to clickable elements.Apply this diff to add proper accessibility attributes:
<ListItem className={itemClass} + role="button" + tabIndex={0} onClick={() => { onClick(normalizedItem); }} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + onClick(normalizedItem); + } + }} style={{ ...style, padding: '0 8px', + cursor: 'pointer', }} data-testid='entity-row'>Alternatively, if
adaptClickHandlerToKeyDownutility is available (mentioned in the AI summary context), use it:<ListItem className={itemClass} + role="button" + tabIndex={0} onClick={() => { onClick(normalizedItem); }} + onKeyDown={adaptClickHandlerToKeyDown(() => { + onClick(normalizedItem); + })} style={{ ...style, padding: '0 8px', + cursor: 'pointer', }} data-testid='entity-row'>packages/javascript/bh-shared-ui/src/components/UpsertSSOProviders/SSOProviderConfigForm.tsx (1)
94-101: Consider parent toggle state for consistent disabled styling.The label's reduced opacity currently only checks
role_provisionvalue (line 97), but doesn't account for when the parent toggle (auto_provision.enabled) disables this entire control (line 86). Ifauto_provision.enabledis false whilerole_provisionis true, the label won't show reduced opacity despite the control being disabled.Apply this diff to ensure the label styling reflects both conditions:
label={ <Typography className={clsx( - `ml-4 ${!watch('config.auto_provision.role_provision') && 'dark:text-white dark:text-opacity-75 text-black text-opacity-75'}` + `ml-4 ${(!watch('config.auto_provision.enabled') || !watch('config.auto_provision.role_provision')) && 'dark:text-white dark:text-opacity-75 text-black text-opacity-75'}` )}> Allow SSO Provider to modify roles </Typography> }
♻️ Duplicate comments (1)
packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.test.tsx (1)
116-119: Same brittleness issue as Line 69-72.This suffers from the same index-based selection fragility. Consider using a more specific selector that identifies the menu trigger by its accessible name or test ID.
🧹 Nitpick comments (10)
packages/javascript/bh-shared-ui/src/components/CheckboxGroup/CheckboxGroup.tsx (1)
40-54: Consider improving grouping semantics for better accessibility.The checkbox group could benefit from stronger semantic association between the title and the checkboxes. Consider one of these approaches:
- Replace
<section>and<h3>with<fieldset>and<legend>- Add
aria-labelledbyto the FormGroup, referencing an id on the headingExample with fieldset/legend:
- <section className={classes.root}> - <h3>{groupTitle}</h3> - <FormGroup> + <fieldset className={classes.root}> + <legend>{groupTitle}</legend> + <FormGroup component="div">packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (1)
3-10: Consider removing redundant runtime check.The check
'key' in eventon Line 5 is redundant since the parameter is typed asKeyboardEvent<HTMLElement>, which always has akeyproperty. This guard is unnecessary.If desired, simplify to:
export function adaptClickHandlerToKeyDown(handler: KeyboardEventHandler<HTMLElement>) { return (event: KeyboardEvent<HTMLElement>) => { - if ('key' in event) { - if (event.key === 'Enter' || event.key === ' ') { - handler(event); - } + if (event.key === 'Enter' || event.key === ' ') { + handler(event); } }; }packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.test.tsx (1)
69-72: Consider using more specific test selectors.Selecting the last button by index is brittle and doesn't clearly express test intent. If the button structure changes, this test could silently pass while testing the wrong element.
Consider using a more specific selector that targets the button's purpose:
-const listItems = screen.getAllByRole('button'); -const lastListItem = screen.getAllByRole('button')[listItems.length - 1]; - -expect(lastListItem).toHaveAttribute('aria-haspopup'); +const menuTrigger = screen.getByRole('button', { name: /menu/i }); +expect(menuTrigger).toHaveAttribute('aria-haspopup');Or use a test ID for the specific menu trigger button if accessible names aren't sufficient.
packages/javascript/bh-shared-ui/src/components/ColumnHeaders/ColumnHeaders.tsx (1)
74-77: Remove redundant role and tabIndex from Button element.Button elements already have an implicit
role='button'and are focusable by default, making these attributes redundant.<Button - role='button' onClick={onSort} - tabIndex={0} onKeyDown={adaptClickHandlerToKeyDown(onSort)}The button will maintain the same behavior without these redundant attributes.
packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/ListItemActionMenu.tsx (1)
64-87: Consider using semantic<button>elements.While the
role='button',tabIndex={0}, andonKeyDownpattern correctly implements keyboard accessibility, semantic<button>elements would be more appropriate here and eliminate the need for manual ARIA roles.If styling constraints allow, refactor to use actual buttons:
-<div - role='button' - tabIndex={0} - onKeyDown={adaptClickHandlerToKeyDown(handleRun)} - className={listItemStyles} - onClick={handleRun}> +<button + type="button" + className={listItemStyles} + onClick={handleRun}> Run -</div> +</button>Apply similar changes to the Edit/Share and Delete actions.
packages/javascript/bh-shared-ui/src/components/ExploreTable/TableControls.tsx (1)
102-110: Inconsistent button implementation.The download control uses a semantic
<button>element (lines 92-99), while expand and close use<div>elements withrole='button'. For consistency and better semantics, consider using actual<button>elements for all controls.Refactor expand and close controls to use semantic buttons:
-<div - role='button' - tabIndex={0} - onClick={onExpandClick} - onKeyDown={adaptClickHandlerToKeyDown(onExpandClick)} - data-testid='expand-button' - aria-label='Expand table view'> +<button + type="button" + onClick={onExpandClick} + data-testid='expand-button' + aria-label='Expand table view'> <FontAwesomeIcon className={ICON_CLASSES} icon={faExpand} /> -</div> +</button>Also applies to: 121-129
packages/javascript/bh-shared-ui/src/components/VirtualizedNodeList.tsx (1)
92-105: Keyboard accessibility implemented, consider semantic button.The row interaction correctly implements keyboard accessibility with
role='button',tabIndex={0}, andonKeyDown. However, for better semantics and reduced ARIA overhead, consider using a<button>element instead of a<div>withrole='button'.If styling constraints permit:
-<div - role='button' - onClick={() => normalizedItem.onClick?.(index)} - onKeyDown={adaptClickHandlerToKeyDown(() => normalizedItem.onClick?.(index))} - tabIndex={0}> +<button + type="button" + className="w-full text-left" + onClick={() => normalizedItem.onClick?.(index)}> <NodeIcon nodeType={normalizedItem.kind} /> <Tooltip tooltip={normalizedItem.name} contentProps={{ className: 'max-w-80 dark:bg-neutral-dark-5 border-0' }}> <div className={cn('truncate ml-2', { 'ml-10': isAssetGroupTagNode(item) })}> {normalizedItem.name} </div> </Tooltip> -</div> +</button>packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx (1)
105-128: Keyboard accessibility implemented with clear ARIA label.The interactive surface correctly implements keyboard support and provides a descriptive aria-label. Consider using a semantic
<button>element instead of a<div>withrole='button'for better native accessibility support.-<div - role='button' - tabIndex={0} - className='w-full h-full' - key={`${id}-${idx}`} - aria-label='Run pre-built search query' - onClick={() => clickHandler(query, id)} - onKeyDown={adaptClickHandlerToKeyDown(() => - clickHandler(query, id) - )}> +<button + type="button" + className='w-full h-full text-left' + key={`${id}-${idx}`} + aria-label='Run pre-built search query' + onClick={() => clickHandler(query, id)}>packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx (1)
104-115: Add explanatory comment to clarifyaria-controls={undefined}intent.The
aria-controls={undefined}on line 112 is necessary and correct—it's a deliberate override to prevent accessibility issues with the default radix/downshift behavior. A similar pattern exists in PrivilegeZones.tsx with the comment: "aria-controls is optional, and default radix prop breaks accessibility" referencing a known Radix UI issue.For consistency and maintainability, add the same explanatory comment above the line:
// per https://github.com/radix-ui/primitives/issues/3013#issuecomment-2453054222 // aria-controls is optional, and default radix prop breaks accessibility aria-controls={undefined}packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx (1)
272-277: Reconsider role='button' semantics for editor focus wrapper.The wrapper div uses
role='button'to make the editor area focusable, but this creates a semantic mismatch. Screen readers will announce this as a "button," yet clicking or activating it only focuses the editor rather than performing a button-like action. This may confuse users expecting button behavior.Consider these alternatives:
- Remove the wrapper and ensure
CypherEditoritself is properly focusable- If a wrapper is needed, use a more semantically appropriate approach (e.g., no explicit role, just tabIndex)
- If keeping role='button', add an aria-label like "Focus editor" to clarify the action
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (97)
.yarn/cache/@bloodhoundenterprise-doodleui-npm-1.0.0-alpha.30-c044da876c-23dadce44e.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@bloodhoundenterprise-doodleui-npm-1.0.0-alpha.31-3831705a03-d8068b2f39.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@eslint-community-eslint-utils-npm-4.9.0-fe45a08548-ae9b98eea0.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@eslint-community-regexpp-npm-4.12.2-3d54624470-1770bc81f6.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@eslint-config-array-npm-0.21.1-c33ed9ec91-fc5b57803b.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@eslint-config-helpers-npm-0.4.1-f92cc14882-8c4c504021.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@eslint-core-npm-0.16.0-ceefcac859-5c08dbf08a.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@eslint-eslintrc-npm-3.3.1-c3967fc0c3-8241f998f0.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@eslint-js-npm-9.38.0-0be8cbf503-24219b9547.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@eslint-object-schema-npm-2.1.7-cb962a5b9b-fc5708f192.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@eslint-plugin-kit-npm-0.4.0-98f470b681-bb82be19c9.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@humanfs-core-npm-0.19.1-e2e7aaeb6e-611e054514.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@humanfs-node-npm-0.16.7-fa16bdb590-7d2a396a94.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@humanwhocodes-retry-npm-0.4.3-a8d7ca1663-d423455b9d.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@types-eslint-plugin-jsx-a11y-npm-6.10.1-7d43a4d716-bc9ecd7d33.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@types-estree-npm-1.0.8-2195bac6d6-bd93e2e415.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/acorn-npm-8.15.0-0764cf600e-309c6b49ae.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/aria-query-npm-5.3.2-78632ac5c5-d971175c85.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/array-buffer-byte-length-npm-1.0.2-c2be1e97e0-0ae3786195.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/array-includes-npm-3.1.9-b081638946-b58dc526fe.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/arraybuffer.prototype.slice-npm-1.0.4-01f62a9713-b1d1fd20be.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/ast-types-flow-npm-0.0.8-d5c457c18e-0a64706609.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/async-function-npm-1.0.0-a81667ebcd-9102e246d1.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/async-generator-function-npm-1.0.0-14cf981d13-74a71a4a2d.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/axe-core-npm-4.11.0-56dfab6db3-57b0d7206d.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/axobject-query-npm-4.1.0-9703554323-7d1e87bf0a.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/damerau-levenshtein-npm-1.0.8-bda7311c69-d240b77575.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/data-view-buffer-npm-1.0.2-93c9247e37-1e1cd509c3.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/data-view-byte-length-npm-1.0.2-96d312fb9c-3600c91ced.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/data-view-byte-offset-npm-1.0.1-315a12a556-8dd492cd51.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/es-abstract-npm-1.24.0-dc8c602e35-06b3d605e5.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/es-to-primitive-npm-1.3.0-470b6d51b6-9669658803.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/eslint-npm-9.38.0-21aed2c277-53b5551e2e.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/eslint-plugin-jsx-a11y-npm-6.10.2-23afcd8d2e-0cc861398f.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/eslint-scope-npm-8.4.0-8ed12feb40-cf88f42cd5.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/eslint-visitor-keys-npm-4.2.1-435d5be22a-3a77e3f99a.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/espree-npm-10.4.0-9633b00e55-5f9d0d7c81.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/esquery-npm-1.6.0-16fee31531-08ec4fe446.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/file-entry-cache-npm-8.0.0-5b09d19a83-f67802d333.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/flat-cache-npm-4.0.1-12bf2455f7-899fc86bf6.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/flatted-npm-3.3.3-ca455563b2-8c96c02fbe.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/function.prototype.name-npm-1.1.8-2cf198aac8-3a366535dc.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/generator-function-npm-2.0.1-aed34a724a-3bf87f7b02.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/get-intrinsic-npm-1.3.1-2f734f40ec-c02b3b6a44.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/get-symbol-description-npm-1.1.0-7a9e0b1c24-655ed04db4.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/globals-npm-14.0.0-5fc3d8d5da-534b821673.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/globalthis-npm-1.0.4-de22ac6193-39ad667ad9.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/has-proto-npm-1.2.0-0108d177d3-f55010cb94.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/internal-slot-npm-1.1.0-269ac0e8be-8e0991c2d0.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/is-array-buffer-npm-3.0.5-8f0828e156-f137a2a6e7.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/is-bigint-npm-1.1.0-963b4e89e1-ee1544f0e6.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/is-boolean-object-npm-1.2.2-ceb8c82b17-0415b181e8.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/is-data-view-npm-1.0.2-8a9e34c5e6-31600dd199.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/is-date-object-npm-1.1.0-c444eba828-d6c36ab9d2.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/is-finalizationregistry-npm-1.1.1-f9cad6c9aa-38c646c506.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/is-map-npm-2.0.3-9e061e76e3-e6ce5f6380.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/is-number-object-npm-1.1.1-010c417fc6-6517f0a0e8.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/is-regex-npm-1.2.1-70a484f2c8-99ee0b6d30.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/is-set-npm-2.0.3-1b72c9a855-36e3f8c44b.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/is-shared-array-buffer-npm-1.0.4-70c977585b-1611fedc17.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/is-string-npm-1.1.1-d2c4f9f448-2eeaaff605.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/is-symbol-npm-1.1.1-f17b666ca9-bfafacf037.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/is-weakmap-npm-2.0.2-ced3cab2dc-f36aef758b.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/is-weakref-npm-1.1.1-e6458807f4-1769b9aed5.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/is-weakset-npm-2.0.4-155b83e84b-5c6c8415a0.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/json-buffer-npm-3.0.1-f8f6d20603-9026b03edc.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/jsx-ast-utils-npm-3.3.5-114c80f97a-f4b05fa4d7.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/keyv-npm-4.5.4-4c8e2cf7f7-74a24395b1.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/language-subtag-registry-npm-0.3.23-06b360f90f-0b64c1a6c5.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/language-tags-npm-1.0.9-3ea51f204b-57c530796d.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/object-inspect-npm-1.13.4-4e741f9806-582810c6a8.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/object.assign-npm-4.1.7-a3464be41b-60e07d2651.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/object.fromentries-npm-2.0.8-8f6e2db04a-29b2207a2d.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/own-keys-npm-1.0.1-1253f9b344-cc9dd7d85c.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/reflect.getprototypeof-npm-1.0.10-8c3ce862a2-ccc5debeb6.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/regexp.prototype.flags-npm-1.5.4-39008ab64c-18cb667e56.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/safe-array-concat-npm-1.1.3-dab0384e54-00f6a68140.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/safe-push-apply-npm-1.0.0-51a0a42944-8c11cbee6d.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/safe-regex-test-npm-1.1.0-453eb81b83-3c809abeb8.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/set-proto-npm-1.0.0-68d7485485-ec27cbbe33.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/side-channel-list-npm-1.0.0-14f74146d1-603b928997.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/side-channel-map-npm-1.0.1-5903573b3c-42501371cd.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/side-channel-npm-1.1.0-4993930974-bf73d6d668.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/side-channel-weakmap-npm-1.0.2-027acaf499-a815c89bc7.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/stop-iteration-iterator-npm-1.1.0-057344287e-be944489d8.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/string.prototype.includes-npm-2.0.1-12fb63787c-ed4b7058b0.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/string.prototype.trim-npm-1.2.10-40a44bc719-87659cd856.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/string.prototype.trimend-npm-1.0.9-e8729528fb-cb86f639f4.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/string.prototype.trimstart-npm-1.0.8-8c6b16ba6e-df1007a7f5.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/typed-array-byte-length-npm-1.0.3-0769937080-cda9352178.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/typed-array-byte-offset-npm-1.0.4-12f60e4553-670b7e6bb1.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/typed-array-length-npm-1.0.7-ac6ef772a7-deb1a4ffdb.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/unbox-primitive-npm-1.1.0-269638c590-729f13b84a.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/which-boxed-primitive-npm-1.1.1-80ca20c912-ee41d0260e.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/which-builtin-type-npm-1.2.1-bbbdf9137f-7a3617ba0e.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/which-collection-npm-1.0.2-0d6277e921-c51821a331.zipis excluded by!**/.yarn/**,!**/*.zipyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (58)
cmd/ui/.eslintrc.cjs(2 hunks)cmd/ui/package.json(3 hunks)cmd/ui/src/components/MainNav/MainNavData.tsx(1 hunks)cmd/ui/src/components/NoDataFileUploadDialogWithLinks.tsx(1 hunks)cmd/ui/src/components/SigmaChart/GraphEdgeEvents.tsx(1 hunks)cmd/ui/src/views/Administration/Administration.test.tsx(1 hunks)cmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.test.tsx(1 hunks)cmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.tsx(1 hunks)packages/javascript/bh-shared-ui/.eslintrc.cjs(2 hunks)packages/javascript/bh-shared-ui/package.json(3 hunks)packages/javascript/bh-shared-ui/src/components/CheckboxGroup/CheckboxGroup.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/CheckboxGroup/CheckboxGroup.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/ColumnHeaders/ColumnHeaders.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/CopyToClipboardButton/CopyToClipboardButton.tsx(3 hunks)packages/javascript/bh-shared-ui/src/components/DateRangeInputs.tsx(0 hunks)packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.test.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTableHeaderCell.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/ExploreTable/ManageColumnsComboBox/ManageColumnsComboBox.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/ExploreTable/ManageColumnsComboBox/ManageColumnsListItem.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/ExploreTable/TableControls.tsx(3 hunks)packages/javascript/bh-shared-ui/src/components/ExploreTable/useExploreTableRowsAndColumns.tsx(3 hunks)packages/javascript/bh-shared-ui/src/components/FileStatusListItem/FileStatusListItem.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/GraphMenu/GraphMenu.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/Icon.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/MenuItem.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/Navigation/AppLink.tsx(3 hunks)packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/Navigation/SubNav.test.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/ListItemActionMenu.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/ListItemActionMenu.tsx(3 hunks)packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.test.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/SSOProviderTable/SSOProviderTable.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/UpsertSSOProviders/SSOProviderConfigForm.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/VirtualizedNodeList.tsx(2 hunks)packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx(1 hunks)packages/javascript/bh-shared-ui/src/utils/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx(7 hunks)packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/EdgeFilter.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/PathfindingSearch.test.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/PathfindingSwapButton.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/SaveQueryActionMenu.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabel.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/Explore/fragments.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Cypher/Cypher.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/EntitySelectorsInformation.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/ObjectCountPanel.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectorsList.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/FilterDialog/FilterDialog.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/HistoryContent.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/Users/UserActionsMenu.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/Users/Users.test.tsx(5 hunks)
💤 Files with no reviewable changes (3)
- packages/javascript/bh-shared-ui/src/components/DateRangeInputs.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectorsList.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx
🧰 Additional context used
🧠 Learnings (23)
📓 Common learnings
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: Icon components in the BloodHound codebase use BaseSVG which automatically includes VisuallyHidden labels with accessible names from the name prop, so additional aria-label/title attributes are not needed on parent elements.
📚 Learning: 2025-08-28T18:42:47.612Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/utils.tsx already provides accessibility through VisuallyHidden labels, eliminating the need for additional accessible names on wrapper elements.
Applied to files:
packages/javascript/bh-shared-ui/src/components/SSOProviderTable/SSOProviderTable.tsxpackages/javascript/bh-shared-ui/src/components/UpsertSSOProviders/SSOProviderConfigForm.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/EdgeFilter.tsxpackages/javascript/bh-shared-ui/src/components/FileStatusListItem/FileStatusListItem.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/PathfindingSwapButton.tsxcmd/ui/src/components/MainNav/MainNavData.tsxpackages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/FilterDialog/FilterDialog.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabel.tsxpackages/javascript/bh-shared-ui/src/components/VirtualizedNodeList.tsxpackages/javascript/bh-shared-ui/src/views/Users/UserActionsMenu.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsxpackages/javascript/bh-shared-ui/src/components/ExploreTable/ManageColumnsComboBox/ManageColumnsComboBox.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/HistoryContent.tsxpackages/javascript/bh-shared-ui/src/components/Icon.tsxpackages/javascript/bh-shared-ui/src/views/Explore/fragments.tsx
📚 Learning: 2025-08-28T18:42:47.612Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in BloodHound's bh-shared-ui package automatically includes VisuallyHidden labels with accessible names using the pattern "app-icon-{name}", making additional aria-label or title attributes unnecessary on parent elements.
Applied to files:
packages/javascript/bh-shared-ui/src/components/SSOProviderTable/SSOProviderTable.tsxpackages/javascript/bh-shared-ui/src/components/Icon.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/CypherSearch.test.tsxcmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Cypher/Cypher.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/PathfindingSearch.test.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/CypherSearch.test.tsxcmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.test.tsxpackages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.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:
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.test.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/EdgeFilter.tsxcmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.test.tsxcmd/ui/src/components/MainNav/MainNavData.tsxcmd/ui/src/components/NoDataFileUploadDialogWithLinks.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/FilterDialog/FilterDialog.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabel.tsxpackages/javascript/bh-shared-ui/src/components/ExploreTable/TableControls.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/SaveQueryActionMenu.tsxpackages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsxpackages/javascript/bh-shared-ui/src/views/Users/UserActionsMenu.tsxpackages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/ListItemActionMenu.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx
📚 Learning: 2025-09-08T19:24:33.396Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.test.tsx:17-20
Timestamp: 2025-09-08T19:24:33.396Z
Learning: In BloodHound codebase, prefer importing `render` and `screen` from custom test-utils (e.g., `import { render, screen } from '../../test-utils';`) instead of directly from `testing-library/react`.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.test.tsxpackages/javascript/bh-shared-ui/src/components/CheckboxGroup/CheckboxGroup.test.tsx
📚 Learning: 2025-08-27T15:10:29.731Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: cmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.tsx:206-210
Timestamp: 2025-08-27T15:10:29.731Z
Learning: In the BloodHound codebase, the team prefers to keep JSX elements without keys in the TabPanels component's tabs array, as indicated by the existing ESLint disable comment and explicit decision to not add keys despite Biome linting warnings.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/ObjectCountPanel.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/HistoryContent.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:
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/EdgeFilter.tsxcmd/ui/src/components/NoDataFileUploadDialogWithLinks.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabel.tsxpackages/javascript/bh-shared-ui/src/components/ExploreTable/TableControls.tsxpackages/javascript/bh-shared-ui/src/components/GraphMenu/GraphMenu.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/SaveQueryActionMenu.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/EntitySelectorsInformation.tsxpackages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/ListItemActionMenu.tsx
📚 Learning: 2025-07-24T18:53:55.326Z
Learnt from: TheNando
Repo: SpecterOps/BloodHound PR: 1720
File: packages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTableDataCell/ExploreTableDataCell.tsx:61-77
Timestamp: 2025-07-24T18:53:55.326Z
Learning: In the BloodHound codebase ExploreTable component, the parent table cell already includes 'group' and 'relative' classes in tableCellProps.className, so child components like ExploreTableDataCell can use group-hover utilities without needing their own group class.
Applied to files:
packages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTableHeaderCell.tsx
📚 Learning: 2025-07-17T16:26:22.455Z
Learnt from: TheNando
Repo: SpecterOps/BloodHound PR: 1693
File: cmd/ui/src/components/SigmaChart/GraphEdgeEvents.tsx:145-152
Timestamp: 2025-07-17T16:26:22.455Z
Learning: In the BloodHound codebase GraphEdgeEvents.tsx, context menu positioning should use event.nativeEvent.offsetX/offsetY (canvas-relative coordinates) rather than event.clientX/clientY (page-relative coordinates). The horizontal offset adjustments are handled later in the processing chain.
Applied to files:
cmd/ui/src/components/SigmaChart/GraphEdgeEvents.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/Explore/ExploreSearch/SavedQueries/TagToZoneLabel.tsxpackages/javascript/bh-shared-ui/src/components/VirtualizedNodeList.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx
📚 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/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabel.tsxpackages/javascript/bh-shared-ui/src/utils/index.tspackages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/HistoryContent.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/EntitySelectorsInformation.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.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/SavedQueries/TagToZoneLabel.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/SaveQueryActionMenu.tsxpackages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.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/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabel.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/SaveQueryActionMenu.tsxpackages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.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/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabel.tsxpackages/javascript/bh-shared-ui/package.json
📚 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/utils/index.ts
📚 Learning: 2025-08-11T23:53:29.554Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1749
File: packages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.tsx:18-20
Timestamp: 2025-08-11T23:53:29.554Z
Learning: In the bh-shared-ui package, js-file-download is configured as an external dependency in rollup, meaning it's expected to be provided by the consuming environment rather than bundled with the library. This is an intentional architectural decision for dependency management.
Applied to files:
packages/javascript/bh-shared-ui/src/utils/index.ts
📚 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:
packages/javascript/bh-shared-ui/src/components/GraphMenu/GraphMenu.tsxpackages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/ListItemActionMenu.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/GraphMenu/GraphMenu.tsx
📚 Learning: 2025-08-27T15:12:49.129Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/hooks/useSavedQueries.ts:98-105
Timestamp: 2025-08-27T15:12:49.129Z
Learning: In the BloodHound codebase, for the `useDeleteQueryPermissions` hook in `packages/javascript/bh-shared-ui/src/hooks/useSavedQueries.ts`, the team prefers to use broad cache invalidation with `queryClient.invalidateQueries(savedQueryKeys.permissions)` rather than targeted invalidation for specific permission entries.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx
📚 Learning: 2025-08-28T18:42:47.612Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: Icon components in the BloodHound codebase use BaseSVG which automatically includes VisuallyHidden labels with accessible names from the name prop, so additional aria-label/title attributes are not needed on parent elements.
Applied to files:
packages/javascript/bh-shared-ui/src/components/Icon.tsx
📚 Learning: 2025-08-27T19:16:31.093Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: cmd/ui/src/App.tsx:99-110
Timestamp: 2025-08-27T19:16:31.093Z
Learning: In BloodHound's web UI, both index.html static favicon links and runtime Helmet favicon updates are required for optimal cross-browser compatibility: index.html provides immediate favicon display on page load to prevent visual glitches, while Helmet enables dynamic favicon updates for theme changes, particularly necessary for Firefox browser support.
Applied to files:
packages/javascript/bh-shared-ui/package.json
🧬 Code graph analysis (21)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Cypher/Cypher.tsx (1)
packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (1)
adaptClickHandlerToKeyDown(3-11)
packages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTableHeaderCell.tsx (2)
packages/javascript/bh-shared-ui/src/utils/theme.ts (1)
cn(41-43)packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (1)
adaptClickHandlerToKeyDown(3-11)
packages/javascript/bh-shared-ui/src/components/ColumnHeaders/ColumnHeaders.tsx (2)
packages/javascript/bh-shared-ui/src/utils/theme.ts (1)
cn(41-43)packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (1)
adaptClickHandlerToKeyDown(3-11)
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabel.tsx (1)
packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (1)
adaptClickHandlerToKeyDown(3-11)
packages/javascript/bh-shared-ui/src/components/VirtualizedNodeList.tsx (1)
packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (1)
adaptClickHandlerToKeyDown(3-11)
packages/javascript/bh-shared-ui/src/components/ExploreTable/ManageColumnsComboBox/ManageColumnsListItem.tsx (2)
packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (1)
adaptClickHandlerToKeyDown(3-11)packages/javascript/bh-shared-ui/src/utils/theme.ts (1)
cn(41-43)
packages/javascript/bh-shared-ui/src/components/ExploreTable/TableControls.tsx (1)
packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (1)
adaptClickHandlerToKeyDown(3-11)
packages/javascript/bh-shared-ui/src/components/GraphMenu/GraphMenu.tsx (1)
packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (1)
adaptClickHandlerToKeyDown(3-11)
packages/javascript/bh-shared-ui/src/components/CheckboxGroup/CheckboxGroup.test.tsx (1)
packages/javascript/bh-shared-ui/src/components/CheckboxGroup/CheckboxGroup.tsx (1)
CheckboxGroupProps(20-27)
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx (2)
packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (1)
adaptClickHandlerToKeyDown(3-11)packages/javascript/bh-shared-ui/src/modules.d.ts (1)
CypherEditor(22-24)
packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx (1)
packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (1)
adaptClickHandlerToKeyDown(3-11)
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/SaveQueryActionMenu.tsx (2)
packages/javascript/bh-shared-ui/src/components/AppIcon/AppIcon.tsx (1)
AppIcon(28-28)packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (1)
adaptClickHandlerToKeyDown(3-11)
cmd/ui/src/views/Administration/Administration.test.tsx (1)
cmd/ui/src/routes/constants.ts (4)
ROUTE_ADMINISTRATION_DATA_QUALITY(33-33)ROUTE_ADMINISTRATION_MANAGE_USERS(35-35)ROUTE_ADMINISTRATION_EARLY_ACCESS_FEATURES(37-37)ROUTE_ADMINISTRATION_SSO_CONFIGURATION(36-36)
packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx (1)
packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (1)
adaptClickHandlerToKeyDown(3-11)
packages/javascript/bh-shared-ui/src/components/MenuItem.tsx (1)
packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (1)
adaptClickHandlerToKeyDown(3-11)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx (1)
packages/javascript/bh-shared-ui/src/routes/index.tsx (3)
zonesPath(17-17)labelsPath(18-18)historyPath(26-26)
packages/javascript/bh-shared-ui/src/components/CopyToClipboardButton/CopyToClipboardButton.tsx (1)
packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (1)
adaptClickHandlerToKeyDown(3-11)
packages/javascript/bh-shared-ui/src/views/Explore/fragments.tsx (1)
packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (1)
adaptClickHandlerToKeyDown(3-11)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/EntitySelectorsInformation.tsx (1)
packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (1)
adaptClickHandlerToKeyDown(3-11)
packages/javascript/bh-shared-ui/src/components/ExploreTable/useExploreTableRowsAndColumns.tsx (1)
packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (1)
adaptClickHandlerToKeyDown(3-11)
packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/ListItemActionMenu.tsx (2)
packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/VerticalEllipsis.tsx (1)
VerticalEllipsis(20-26)packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (1)
adaptClickHandlerToKeyDown(3-11)
| onMouseDown={handleEdgeEvents} | ||
| onWheel={handleEdgeEvents} | ||
| onMouseOut={handleEdgeEvents} | ||
| onBlur={handleEdgeEvents} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
Verify the blur handler's effectiveness.
Adding onBlur={handleEdgeEvents} to the canvas element may not have any effect unless the canvas is made focusable (typically via tabIndex). Additionally, the handleEdgeEvents function doesn't appear to have specific handling for blur events based on the visible code.
Please verify:
- Is the canvas element made focusable elsewhere in the code (e.g., via tabIndex)?
- What is the intended behavior for blur events on this canvas?
- Should
handleEdgeEventsinclude specific logic for blur events?
Run the following to check if tabIndex is set on this canvas or in related components:
🏁 Script executed:
#!/bin/bash
# Search for tabIndex on canvas elements in the SigmaChart directory
rg -n "tabIndex" --type=tsx --type=ts -g "SigmaChart/**"
# Search for edge-events canvas configuration
rg -n "edge-events" --type=tsx --type=ts -C 5Length of output: 55
I need to adjust the search strategy since the file type flags aren't recognized. Let me search using glob patterns instead.
🏁 Script executed:
#!/bin/bash
# Find SigmaChart files and their structure
echo "=== Locating SigmaChart files ==="
fd -e tsx -e ts . | grep -i sigmachart | head -20
# Search for tabIndex in SigmaChart files using glob patterns
echo "=== Searching for tabIndex in SigmaChart ==="
rg -n "tabIndex" -g "SigmaChart/**" 2>/dev/null || rg -n "tabIndex" . | grep -i sigmachart
# Search for edge-events configuration
echo "=== Searching for edge-events ==="
rg -n "edge-events" -C 5 . 2>/dev/null | head -40
# Search for handleEdgeEvents function definition
echo "=== Searching for handleEdgeEvents function ==="
rg -n "handleEdgeEvents" -C 8 .Length of output: 5833
Let me examine the full handleEdgeEvents function implementation to understand the complete picture:
🏁 Script executed:
#!/bin/bash
# Get the complete handleEdgeEvents function (lines 43-165)
echo "=== Full handleEdgeEvents function implementation ==="
sed -n '43,165p' ./cmd/ui/src/components/SigmaChart/GraphEdgeEvents.tsxLength of output: 5885
Remove ineffective onBlur handler from the canvas element.
The canvas element lacks a tabIndex attribute and is therefore not focusable. Since canvas elements cannot receive focus, blur events will never fire on this element, making the onBlur={handleEdgeEvents} handler at line 184 ineffective. Additionally, the handleEdgeEvents function only processes click and mousemove events (line 47 condition); blur events would have no specific handling logic.
Remove onBlur={handleEdgeEvents} from the canvas element.
🤖 Prompt for AI Agents
In cmd/ui/src/components/SigmaChart/GraphEdgeEvents.tsx around line 184, the
canvas has an onBlur={handleEdgeEvents} handler which is ineffective because the
canvas has no tabIndex and cannot receive focus so blur will never fire, and
handleEdgeEvents only handles click and mousemove events; remove the onBlur prop
from the canvas element to eliminate dead code and rely on the existing mouse
event handlers instead.
Contributor
Author
There was a problem hiding this comment.
This change was requested by and satisfied axe dev tools, which is my source of truth for this project.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Axe DevTools canvas element blur event accessibility requirement
💡 Result:
Short answer: axe (and related a11y tooling) flags a canvas with a blur/focus handler because a non-interactive element that has interactive handlers must be made properly interactive — i.e. focusable and given an appropriate role/name — or the handler must be removed. This is to meet ARIA/WCAG expectations for keyboard and assistive‑technology behavior.
Key references:
- axe/axe-core uses rules tied to WCAG/ARIA; interactive handlers on non‑interactive elements are considered accessibility issues. [1]
- The widely used jsx‑a11y rules state that non‑interactive elements with mouse/keyboard/focus handlers (e.g., onBlur/onFocus) must have an ARIA role or otherwise be made interactive (or the handler moved to a truly interactive element). [2]
- HTML/tabindex guidance: elements (like canvas) are not focusable by default — you must set tabindex (and ensure semantics/keyboard support) if you want it to be focusable. [3]
Remediation (concise):
- If the canvas is meant to be interactive: add tabindex="0", give it an appropriate ARIA role/name (and implement full keyboard support and accessible name/description), and ensure the behavior matches the role. Cite keyboard instructions in UI or expose controls in DOM.
- If it isn’t meant to be interactive: remove onblur/onfocus handlers (attach handlers to an appropriate interactive control or container), or keep it non-focusable (no tabindex) so blur won’t fire.
Sources:
[1] axe-core rule/tag docs — axe/Deque. (deque.com)
[2] eslint-plugin-jsx-a11y (rules: no-noninteractive-element-interactions / no-static-element-interactions). (github.com)
[3] HTML tabindex / focusability guidance. (w3.org)
Citations:
- 1: https://www.deque.com/axe/core-documentation/api-documentation/?utm_source=openai
- 2: https://github.com/jsx-eslint/eslint-plugin-jsx-a11y?utm_source=openai
- 3: https://www.w3.org/TR/2018/WD-html53-20180426/editing.html?utm_source=openai
@dcairnsspecterops, thank you for clarifying your testing process. I understand that Axe DevTools guided this change and you've verified it satisfies the tool's requirements.
For context: blur events on canvas elements only fire when the element is focusable (typically via tabIndex), and Axe/accessibility tooling generally expects interactive handlers to be on properly interactive elements with appropriate ARIA roles and keyboard support. Since this canvas doesn't have tabIndex, the blur event won't fire in practice—though if Axe DevTools is satisfied with the current implementation, that aligns with your project's accessibility goals.
I'll defer to your judgment on this since you're using Axe DevTools as your source of truth for this PR. Feel free to proceed as you see fit.
🧠 Learnings used
Learnt from: TheNando
Repo: SpecterOps/BloodHound PR: 1693
File: cmd/ui/src/components/SigmaChart/GraphEdgeEvents.tsx:145-152
Timestamp: 2025-07-17T16:26:22.455Z
Learning: In the BloodHound codebase GraphEdgeEvents.tsx, context menu positioning should use event.nativeEvent.offsetX/offsetY (canvas-relative coordinates) rather than event.clientX/clientY (page-relative coordinates). The horizontal offset adjustments are handled later in the processing chain.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: Icon components in the BloodHound codebase use BaseSVG which automatically includes VisuallyHidden labels with accessible names from the name prop, so additional aria-label/title attributes are not needed on parent elements.
packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx
Outdated
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/components/ExploreTable/useExploreTableRowsAndColumns.tsx
Outdated
Show resolved
Hide resolved
| itemCount={itemCount} | ||
| itemData={itemData} | ||
| itemSize={ITEM_SIZE} | ||
| outerElementType='summary' |
Contributor
There was a problem hiding this comment.
Invalid semantic HTML: <summary> element misused.
The <summary> element should only be used as a direct child of a <details> element for disclosure widgets. Using it as the outer container for a scrollable list violates HTML semantics and will confuse screen readers.
Apply this diff to use a semantically appropriate element:
- outerElementType='summary'
+ outerElementType='div'📝 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
| outerElementType='summary' | |
| outerElementType='div' |
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx
around line 174, the prop outerElementType='summary' misuses the <summary>
element; replace it with a semantically appropriate container (e.g. 'div') — or
'ul' if the inner items are list items — by setting outerElementType='div' (or
'ul') and, if you choose a non-list container, add role="list"/aria-label as
needed to preserve accessibility semantics.
Contributor
Author
There was a problem hiding this comment.
This chang satisfied axe dev tools, which is my source of truth for this project.
Contributor
There was a problem hiding this comment.
@dcairnsspecterops, understood! If Axe DevTools is passing with this configuration, I'll defer to that. Thank you for clarifying your approach to accessibility validation in this project.
🐰✨
✏️ Learnings added
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx:174-174
Timestamp: 2025-11-06T21:31:03.107Z
Learning: In the BloodHound project, Axe DevTools is the source of truth for accessibility decisions. If a change satisfies Axe DevTools requirements, it should be accepted even if it might appear to violate traditional semantic HTML practices.
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: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: Icon components in the BloodHound codebase use BaseSVG which automatically includes VisuallyHidden labels with accessible names from the name prop, so additional aria-label/title attributes are not needed on parent elements.
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.670Z
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.
packages/javascript/bh-shared-ui/src/components/Navigation/AppLink.tsx
Outdated
Show resolved
Hide resolved
| if (discardQueryParams) { | ||
| return ( | ||
| <Link to={to} {...props}> | ||
| <Link to={to} aria-label={`Navigate to ${path}`} {...props}> |
Contributor
There was a problem hiding this comment.
Remove aria-label from links with text content.
Adding aria-label to links that already have text content (children) completely replaces the visible link text for screen readers. Instead of hearing descriptive text like "User Management" or "API Keys", screen reader users will hear the less meaningful path like "Navigate to /administration/users". This degrades the user experience and contradicts accessibility best practices.
Links derive their accessible names from their text content by default. Only add aria-label to links that lack text content (e.g., icon-only links).
Apply this diff to remove the redundant aria-labels:
if (discardQueryParams) {
return (
- <Link to={to} aria-label={`Navigate to ${path}`} {...props}>
+ <Link to={to} {...props}>
{children}
</Link>
);
}
const search = persistSearchParams(GloballySupportedSearchParams);
const toWithParams = applyPreservedParams(to, search);
return (
- <Link to={toWithParams} aria-label={`Navigate to ${path}`} {...props}>
+ <Link to={toWithParams} {...props}>
{children}
</Link>
);Also applies to: 40-40
🤖 Prompt for AI Agents
In packages/javascript/bh-shared-ui/src/components/Navigation/AppLink.tsx around
lines 30 and 40, remove the hardcoded aria-label on Link elements that have
visible text children; accessible name should come from link text by default.
Instead, conditionally set aria-label only when the link has no text content
(e.g., icon-only links) by checking for empty or non-string children and
providing an appropriate aria-label in that case. Ensure you do not leave
redundant aria-label attributes on links with readable children and update both
occurrences referenced.
Contributor
Author
There was a problem hiding this comment.
This is a fallback, since Children may be something other than a string
Contributor
There was a problem hiding this comment.
@dcairnsspecterops, I understand your reasoning about handling non-string children. However, the current implementation applies the aria-label to all links, which will override readable text content for screen readers when text children are present.
A more targeted approach would be to conditionally apply the aria-label only when the link lacks readable text content:
export const AppLink = ({ children, to, discardQueryParams, ...props }: LinkProps & AppNavigateProps) => {
const path = typeof to === 'string' ? to : (to as Partial<Path>).pathname;
// Only add aria-label for non-textual children (icons, complex JSX, etc.)
const hasTextContent = typeof children === 'string' ||
(typeof children === 'number') ||
(Array.isArray(children) && children.some(child => typeof child === 'string' || typeof child === 'number'));
const ariaLabel = hasTextContent ? undefined : `Navigate to ${path}`;
if (discardQueryParams) {
return (
<Link to={to} aria-label={ariaLabel} {...props}>
{children}
</Link>
);
}
const search = persistSearchParams(GloballySupportedSearchParams);
const toWithParams = applyPreservedParams(to, search);
return (
<Link to={toWithParams} aria-label={ariaLabel} {...props}>
{children}
</Link>
);
};This way:
- Links with text children get their accessible name from the text (better UX)
- Icon-only or non-textual links get the fallback aria-label you intended
- Screen readers won't have readable text replaced by generic paths
Since Axe DevTools is your source of truth for accessibility (per your earlier guidance), does the current implementation pass Axe validation on pages where AppLink is used with text content?
✏️ Learnings added
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.707Z
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.
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
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx:0-0
Timestamp: 2025-11-06T21:23:30.670Z
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.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/utils.tsx already provides accessibility through VisuallyHidden labels, eliminating the need for additional accessible names on wrapper elements.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: Icon components in the BloodHound codebase use BaseSVG which automatically includes VisuallyHidden labels with accessible names from the name prop, so additional aria-label/title attributes are not needed on parent elements.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in BloodHound's bh-shared-ui package automatically includes VisuallyHidden labels with accessible names using the pattern "app-icon-{name}", making additional aria-label or title attributes unnecessary on parent elements.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in BloodHound's bh-shared-ui package automatically includes VisuallyHidden labels with accessible names using the pattern "app-icon-{name}", making additional aria-label or title attributes unnecessary on parent elements.
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx:174-174
Timestamp: 2025-11-06T21:31:03.107Z
Learning: In the BloodHound project, Axe DevTools is the source of truth for accessibility decisions. If a change satisfies Axe DevTools requirements, it should be accepted even if it might appear to violate traditional semantic HTML practices.
packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
86
to
90
| useLayoutEffect(() => { | ||
| if (cypherEditorRef.current?.cypherEditor) { | ||
| cypherEditorRef.current?.cypherEditor?.codemirror?.contentDOM?.setAttribute('aria-label', 'Cypher Editor'); | ||
| } | ||
| }, []); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove redundant useLayoutEffect - prefer the aria-label prop.
This useLayoutEffect directly manipulates the CypherEditor's internal contentDOM to set an aria-label, but line 296 already passes aria-label='Cypher editor' as a prop to CypherEditor. This creates redundancy and potential conflicts. Additionally:
- The casing differs: "Cypher Editor" here vs. "Cypher editor" at line 296
- Direct DOM manipulation of
contentDOMis fragile and tightly couples to CypherEditor's internal implementation
Remove the useLayoutEffect and rely on the prop-based approach at line 296:
- useLayoutEffect(() => {
- if (cypherEditorRef.current?.cypherEditor) {
- cypherEditorRef.current?.cypherEditor?.codemirror?.contentDOM?.setAttribute('aria-label', 'Cypher Editor');
- }
- }, []);
-Also remove the now-unused import:
-import { useEffect, useLayoutEffect, useRef, useState } from 'react';
+import { useEffect, useRef, useState } from 'react';Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx
around lines 86 to 90, remove the useLayoutEffect that directly sets aria-label
on cypherEditorRef.current.cypherEditor.codemirror.contentDOM (it's redundant,
fragile, and conflicts with the aria-label prop passed to CypherEditor at line
296); delete the entire useLayoutEffect block and also remove any now-unused
imports related to useLayoutEffect (or the specific hook import) so the
component relies solely on the aria-label prop for accessibility.
Contributor
Author
There was a problem hiding this comment.
That prop was not finding its was to a satisfactory DOM element, per axe dev tools.
Contributor
There was a problem hiding this comment.
@dcairnsspecterops, thank you for clarifying! That makes sense. If the aria-label prop on the CypherEditor component isn't being applied to the correct DOM element for Axe DevTools, then the direct DOM manipulation via useLayoutEffect is a necessary workaround.
As noted in the project's accessibility standards, Axe DevTools is the source of truth for these decisions. Since this approach satisfies Axe's requirements, the implementation is appropriate even though it involves accessing internal component structure.
✏️ Learnings added
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.086Z
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.
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: 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.
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.
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.
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.670Z
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.
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.
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.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: Icon components in the BloodHound codebase use BaseSVG which automatically includes VisuallyHidden labels with accessible names from the name prop, so additional aria-label/title attributes are not needed on parent elements.
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx:174-174
Timestamp: 2025-11-06T21:31:03.107Z
Learning: In the BloodHound project, Axe DevTools is the source of truth for accessibility decisions. If a change satisfies Axe DevTools requirements, it should be accepted even if it might appear to violate traditional semantic HTML practices.
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+126
to
+131
| <div | ||
| onClick={setFocusOnCypherEditor} | ||
| onKeyDown={adaptClickHandlerToKeyDown(setFocusOnCypherEditor)} | ||
| tabIndex={0} | ||
| className='flex-1' | ||
| role='textbox'> |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove role='textbox' from the wrapper div.
The wrapper div should not have role='textbox' since it's not the actual text input—the CypherEditor component inside it is. The wrapper is merely a click target that delegates focus to the real editor.
Remove the role attribute from the wrapper:
<div
onClick={setFocusOnCypherEditor}
onKeyDown={adaptClickHandlerToKeyDown(setFocusOnCypherEditor)}
tabIndex={0}
- className='flex-1'
- role='textbox'>
+ className='flex-1'>The CypherEditor component should handle its own accessibility semantics. If additional keyboard affordances are needed for focusing the editor, consider using role='button' with an appropriate aria-label like "Focus Cypher editor" instead.
📝 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
| <div | |
| onClick={setFocusOnCypherEditor} | |
| onKeyDown={adaptClickHandlerToKeyDown(setFocusOnCypherEditor)} | |
| tabIndex={0} | |
| className='flex-1' | |
| role='textbox'> | |
| <div | |
| onClick={setFocusOnCypherEditor} | |
| onKeyDown={adaptClickHandlerToKeyDown(setFocusOnCypherEditor)} | |
| tabIndex={0} | |
| className='flex-1'> |
🤖 Prompt for AI Agents
In packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Cypher/Cypher.tsx
around lines 126 to 131, the wrapper div incorrectly has role='textbox' even
though the real text input is the inner CypherEditor; remove the role='textbox'
attribute from this wrapper and let CypherEditor manage its own accessibility
semantics, and if you need explicit keyboard affordances on the wrapper replace
role='textbox' with role='button' plus an appropriate aria-label (e.g.,
aria-label="Focus Cypher editor") and ensure the onKeyDown handler still
delegates focus to the editor.
Contributor
Author
There was a problem hiding this comment.
The editor does act like a textbox, since the user types cypher into it.
Contributor
There was a problem hiding this comment.
@dcairnsspecterops, I understand your point that the editor is used for text input. However, my concern is that the wrapper div itself isn't the actual text input—it's just a click target that delegates focus to the CypherEditor component inside.
According to ARIA specifications, elements with role='textbox' should have contenteditable="true" to be properly editable. The wrapper div lacks this attribute and doesn't directly handle text input; it only focuses the real editor inside when clicked.
That said, I know from our previous discussions that Axe DevTools is the source of truth for accessibility decisions in the BloodHound project. Does this approach satisfy Axe DevTools validation? If Axe is happy with role='textbox' on the wrapper, then I'm happy to defer to that assessment.
🧠 Learnings used
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.086Z
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.
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.
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.
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.670Z
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.
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx:174-174
Timestamp: 2025-11-06T21:31:03.107Z
Learning: In the BloodHound project, Axe DevTools is the source of truth for accessibility decisions. If a change satisfies Axe DevTools requirements, it should be accepted even if it might appear to violate traditional semantic HTML practices.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: Icon components in the BloodHound codebase use BaseSVG which automatically includes VisuallyHidden labels with accessible names from the name prop, so additional aria-label/title attributes are not needed on parent elements.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/FilterDialog/FilterDialog.tsx (1)
80-92: Good addition for accessibility!Adding
aria-label='Filter'provides an accessible name for screen readers, which is essential for this icon-only button. This follows accessibility best practices.Minor optional note: There's a slight text inconsistency between the aria-label ("Filter") and the tooltip ("Filters"). While not critical, aligning them could provide a more consistent experience.
If you prefer consistency, consider:
- aria-label='Filter' + aria-label='Filters'Or update the tooltip to match:
- <Tooltip tooltip='Filters'> + <Tooltip tooltip='Filter'>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/EntitySelectorsInformation.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectorsList.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/FilterDialog/FilterDialog.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/HistoryContent.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectorsList.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/HistoryContent.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/EntitySelectorsInformation.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: Icon components in the BloodHound codebase use BaseSVG which automatically includes VisuallyHidden labels with accessible names from the name prop, so additional aria-label/title attributes are not needed on parent elements.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/utils.tsx already provides accessibility through VisuallyHidden labels, eliminating the need for additional accessible names on wrapper elements.
📚 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:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/FilterDialog/FilterDialog.tsx
📚 Learning: 2025-08-28T18:42:47.612Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/utils.tsx already provides accessibility through VisuallyHidden labels, eliminating the need for additional accessible names on wrapper elements.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/FilterDialog/FilterDialog.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (2)
5-5: Remove redundant runtime check.The check
'key' in eventis unnecessary sinceKeyboardEventalways has akeyproperty defined in the TypeScript type.Apply this diff:
export function adaptClickHandlerToKeyDown(handler: KeyboardEventHandler<HTMLElement>) { return (event: KeyboardEvent<HTMLElement>) => { - if ('key' in event) { - if (event.key === 'Enter' || event.key === ' ') { - handler(event); - } + if (event.key === 'Enter' || event.key === ' ') { + handler(event); } }; }
6-7: Prevent default behavior for Space key.Without calling
preventDefault(), the Space key will both trigger the handler and scroll the page, which can create a confusing user experience when the element is focused.Apply this diff:
export function adaptClickHandlerToKeyDown(handler: KeyboardEventHandler<HTMLElement>) { return (event: KeyboardEvent<HTMLElement>) => { if ('key' in event) { if (event.key === 'Enter' || event.key === ' ') { + event.preventDefault(); handler(event); } } }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/javascript/bh-shared-ui/src/components/Navigation/AppLink.tsx(3 hunks)packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/javascript/bh-shared-ui/src/components/Navigation/AppLink.tsx
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
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.086Z
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.
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx:174-174
Timestamp: 2025-11-06T21:31:03.107Z
Learning: In the BloodHound project, Axe DevTools is the source of truth for accessibility decisions. If a change satisfies Axe DevTools requirements, it should be accepted even if it might appear to violate traditional semantic HTML practices.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: Icon components in the BloodHound codebase use BaseSVG which automatically includes VisuallyHidden labels with accessible names from the name prop, so additional aria-label/title attributes are not needed on parent elements.
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.670Z
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.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/utils.tsx already provides accessibility through VisuallyHidden labels, eliminating the need for additional accessible names on wrapper elements.
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: cmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.tsx:206-210
Timestamp: 2025-08-27T15:10:29.731Z
Learning: In the BloodHound codebase, the team prefers to keep JSX elements without keys in the TabPanels component's tabs array, as indicated by the existing ESLint disable comment and explicit decision to not add keys despite Biome linting warnings.
📚 Learning: 2025-11-06T21:35:45.086Z
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.086Z
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/CypherSearch.tsxpackages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.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/CypherSearch.tsx
📚 Learning: 2025-11-06T21:23:30.670Z
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.670Z
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/CypherSearch.tsxpackages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.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/CypherSearch.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/CypherSearch.tsx
📚 Learning: 2025-11-06T21:35:02.707Z
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.707Z
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/CypherSearch.tsx
📚 Learning: 2025-08-28T18:42:47.612Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/utils.tsx already provides accessibility through VisuallyHidden labels, eliminating the need for additional accessible names on wrapper elements.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsxpackages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx
📚 Learning: 2025-08-28T18:42:47.612Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: Icon components in the BloodHound codebase use BaseSVG which automatically includes VisuallyHidden labels with accessible names from the name prop, so additional aria-label/title attributes are not needed on parent elements.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx
📚 Learning: 2025-08-28T18:42:47.612Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in BloodHound's bh-shared-ui package automatically includes VisuallyHidden labels with accessible names using the pattern "app-icon-{name}", making additional aria-label or title attributes unnecessary on parent elements.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.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:
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx
📚 Learning: 2025-11-06T21:31:03.107Z
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx:174-174
Timestamp: 2025-11-06T21:31:03.107Z
Learning: In the BloodHound project, Axe DevTools is the source of truth for accessibility decisions. If a change satisfies Axe DevTools requirements, it should be accepted even if it might appear to violate traditional semantic HTML practices.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.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/views/Explore/ExploreSearch/CypherSearch.tsx
📚 Learning: 2025-08-27T15:10:29.731Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: cmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.tsx:206-210
Timestamp: 2025-08-27T15:10:29.731Z
Learning: In the BloodHound codebase, the team prefers to keep JSX elements without keys in the TabPanels component's tabs array, as indicated by the existing ESLint disable comment and explicit decision to not add keys despite Biome linting warnings.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx
📚 Learning: 2025-08-27T15:12:49.129Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/hooks/useSavedQueries.ts:98-105
Timestamp: 2025-08-27T15:12:49.129Z
Learning: In the BloodHound codebase, for the `useDeleteQueryPermissions` hook in `packages/javascript/bh-shared-ui/src/hooks/useSavedQueries.ts`, the team prefers to use broad cache invalidation with `queryClient.invalidateQueries(savedQueryKeys.permissions)` rather than targeted invalidation for specific permission entries.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx
🧬 Code graph analysis (1)
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx (2)
packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (1)
adaptClickHandlerToKeyDown(3-11)packages/javascript/bh-shared-ui/src/modules.d.ts (1)
CypherEditor(22-24)
⏰ 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: build-ui
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (2)
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx (2)
86-90: Direct DOM manipulation is necessary here.This
useLayoutEffectapproach is correct and required because thearia-labelprop passed toCypherEditordoesn't propagate to the appropriate DOM element for Axe DevTools validation.Based on learnings.
336-340: LGTM: aria-label correctly placed and onClick simplified.The
aria-labelis now on the Button element (not the nested<p>tag), which correctly follows WCAG 2.5.3 Label in Name. The onClick simplification from arrow function to direct reference is also a nice improvement.
Comment on lines
+272
to
+277
| <div | ||
| role='button' | ||
| tabIndex={0} | ||
| onKeyDown={adaptClickHandlerToKeyDown(setFocusOnCypherEditor)} | ||
| onClick={setFocusOnCypherEditor} | ||
| className='flex-1'> |
Contributor
There was a problem hiding this comment.
Add accessible name to the focusable wrapper.
The wrapper div has role="button" and tabIndex={0} but is missing an accessible name (aria-label or aria-labelledby). Per WCAG 4.1.2, all interactive elements must have accessible names that describe their purpose.
Apply this diff to add an appropriate label:
<div
role='button'
tabIndex={0}
onKeyDown={adaptClickHandlerToKeyDown(setFocusOnCypherEditor)}
onClick={setFocusOnCypherEditor}
+ aria-label='Focus Cypher editor'
className='flex-1'>📝 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
| <div | |
| role='button' | |
| tabIndex={0} | |
| onKeyDown={adaptClickHandlerToKeyDown(setFocusOnCypherEditor)} | |
| onClick={setFocusOnCypherEditor} | |
| className='flex-1'> | |
| <div | |
| role='button' | |
| tabIndex={0} | |
| onKeyDown={adaptClickHandlerToKeyDown(setFocusOnCypherEditor)} | |
| onClick={setFocusOnCypherEditor} | |
| aria-label='Focus Cypher editor' | |
| className='flex-1'> |
🤖 Prompt for AI Agents
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx
around lines 272-277: the focusable wrapper div has role="button" and
tabIndex={0} but no accessible name; add an aria-label (or aria-labelledby
pointing to an existing visible label id) that succinctly describes the action
(e.g., "Focus Cypher editor" or a localized string) so the element has an
accessible name per WCAG; update the div to include the chosen aria attribute
and ensure any label id referenced exists and is unique.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx(4 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectorsList.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectorsList.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx
- packages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.test.tsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.086Z
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.
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx:174-174
Timestamp: 2025-11-06T21:31:03.107Z
Learning: In the BloodHound project, Axe DevTools is the source of truth for accessibility decisions. If a change satisfies Axe DevTools requirements, it should be accepted even if it might appear to violate traditional semantic HTML practices.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: Icon components in the BloodHound codebase use BaseSVG which automatically includes VisuallyHidden labels with accessible names from the name prop, so additional aria-label/title attributes are not needed on parent elements.
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.670Z
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.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/utils.tsx already provides accessibility through VisuallyHidden labels, eliminating the need for additional accessible names on wrapper elements.
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: cmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.tsx:206-210
Timestamp: 2025-08-27T15:10:29.731Z
Learning: In the BloodHound codebase, the team prefers to keep JSX elements without keys in the TabPanels component's tabs array, as indicated by the existing ESLint disable comment and explicit decision to not add keys despite Biome linting warnings.
📚 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/TagList.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/views/PrivilegeZones/Details/TagList.tsx
📚 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/views/PrivilegeZones/Details/TagList.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: build-ui
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (3)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx (1)
28-28: LGTM! Non-functional import reordering.This is a cosmetic change that reorders the import statement, likely enforced by ESLint rules. No behavioral impact.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx (2)
82-85: LGTM!The shift from role-based queries to test-id queries is consistent with the component changes that now use
data-testidattributes on selector and member rows.
184-200: LGTM!The test correctly handles the rerendered lists by re-querying with the updated test-id selectors. The async query pattern is consistent throughout this section.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/components/GraphControls/GraphControls.tsx (1)
169-169: Verify that aria-label is effective without a semantic role.The
aria-labelattribute on thePoppercomponent (which renders as adivby default) may not be announced by screen readers unless the element has a semantic role. For a popup search interface like this, consider addingrole="region"orrole="dialog"to make the label accessible.Please verify that Axe DevTools accepts this implementation and that screen readers properly announce the label. If not, apply this enhancement:
<Popper open={isCurrentSearchOpen} anchorEl={currentSearchAnchorElement.current} placement='top' disablePortal - aria-label='Search Current Nodes' + role="region" + aria-label="Search Current Nodes" className='w-[90%] z-[1]'>Based on learnings: Axe DevTools is the source of truth for accessibility decisions in this codebase, so if this satisfies Axe requirements, it may be acceptable as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cmd/ui/package.json(4 hunks)packages/javascript/bh-shared-ui/package.json(4 hunks)packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx(3 hunks)packages/javascript/bh-shared-ui/src/components/ExploreTable/ManageColumnsComboBox/ManageColumnsComboBox.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/GraphControls/GraphControls.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/GraphMenu/GraphMenu.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/SearchResultItem/SearchResultItem.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/javascript/bh-shared-ui/src/components/ExploreTable/ManageColumnsComboBox/ManageColumnsComboBox.tsx
- cmd/ui/package.json
🧰 Additional context used
🧠 Learnings (19)
📓 Common learnings
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.086Z
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.
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx:174-174
Timestamp: 2025-11-06T21:31:03.107Z
Learning: In the BloodHound project, Axe DevTools is the source of truth for accessibility decisions. If a change satisfies Axe DevTools requirements, it should be accepted even if it might appear to violate traditional semantic HTML practices.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: Icon components in the BloodHound codebase use BaseSVG which automatically includes VisuallyHidden labels with accessible names from the name prop, so additional aria-label/title attributes are not needed on parent elements.
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.670Z
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.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/utils.tsx already provides accessibility through VisuallyHidden labels, eliminating the need for additional accessible names on wrapper elements.
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: cmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.tsx:206-210
Timestamp: 2025-08-27T15:10:29.731Z
Learning: In the BloodHound codebase, the team prefers to keep JSX elements without keys in the TabPanels component's tabs array, as indicated by the existing ESLint disable comment and explicit decision to not add keys despite Biome linting warnings.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in BloodHound's bh-shared-ui package automatically includes VisuallyHidden labels with accessible names using the pattern "app-icon-{name}", making additional aria-label or title attributes unnecessary on parent elements.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in BloodHound's bh-shared-ui package automatically includes VisuallyHidden labels with accessible names using the pattern "app-icon-{name}", making additional aria-label or title attributes unnecessary on parent elements.
📚 Learning: 2025-11-06T21:35:45.086Z
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.086Z
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/components/GraphControls/GraphControls.tsxpackages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsxpackages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx
📚 Learning: 2025-11-06T21:23:30.670Z
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.670Z
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/components/GraphControls/GraphControls.tsxpackages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.tsxpackages/javascript/bh-shared-ui/src/components/SearchResultItem/SearchResultItem.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsxpackages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx
📚 Learning: 2025-08-28T18:42:47.612Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/utils.tsx already provides accessibility through VisuallyHidden labels, eliminating the need for additional accessible names on wrapper elements.
Applied to files:
packages/javascript/bh-shared-ui/src/components/GraphControls/GraphControls.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsxpackages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx
📚 Learning: 2025-11-06T21:35:02.707Z
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.707Z
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/components/GraphControls/GraphControls.tsxpackages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.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:
packages/javascript/bh-shared-ui/src/components/GraphControls/GraphControls.tsxpackages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsxpackages/javascript/bh-shared-ui/src/components/GraphMenu/GraphMenu.tsxpackages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.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/components/SearchCurrentNodes/SearchCurrentNodes.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsxpackages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx
📚 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/views/PrivilegeZones/Details/SearchBar.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-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/package.json
📚 Learning: 2025-08-27T19:16:31.093Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: cmd/ui/src/App.tsx:99-110
Timestamp: 2025-08-27T19:16:31.093Z
Learning: In BloodHound's web UI, both index.html static favicon links and runtime Helmet favicon updates are required for optimal cross-browser compatibility: index.html provides immediate favicon display on page load to prevent visual glitches, while Helmet enables dynamic favicon updates for theme changes, particularly necessary for Firefox browser support.
Applied to files:
packages/javascript/bh-shared-ui/package.json
📚 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:
packages/javascript/bh-shared-ui/src/components/GraphMenu/GraphMenu.tsx
📚 Learning: 2025-07-17T16:26:22.455Z
Learnt from: TheNando
Repo: SpecterOps/BloodHound PR: 1693
File: cmd/ui/src/components/SigmaChart/GraphEdgeEvents.tsx:145-152
Timestamp: 2025-07-17T16:26:22.455Z
Learning: In the BloodHound codebase GraphEdgeEvents.tsx, context menu positioning should use event.nativeEvent.offsetX/offsetY (canvas-relative coordinates) rather than event.clientX/clientY (page-relative coordinates). The horizontal offset adjustments are handled later in the processing chain.
Applied to files:
packages/javascript/bh-shared-ui/src/components/GraphMenu/GraphMenu.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.tsx
📚 Learning: 2025-08-07T19:30:58.455Z
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 1749
File: packages/javascript/bh-shared-ui/src/views/Explore/fragments.tsx:107-117
Timestamp: 2025-08-07T19:30:58.455Z
Learning: In the BloodHound codebase Field component in packages/javascript/bh-shared-ui/src/views/Explore/fragments.tsx, null values are intentionally NOT explicitly filtered out in the value validation check. The EntityField type definition does not include null as a supported value type, and the try-catch block is used to safely handle edge cases where Object.keys() might be called on null values while preserving the intended null value handling behavior.
Applied to files:
packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.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/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx
📚 Learning: 2025-11-06T21:31:03.107Z
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx:174-174
Timestamp: 2025-11-06T21:31:03.107Z
Learning: In the BloodHound project, Axe DevTools is the source of truth for accessibility decisions. If a change satisfies Axe DevTools requirements, it should be accepted even if it might appear to violate traditional semantic HTML practices.
Applied to files:
packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx
📚 Learning: 2025-08-28T18:42:47.612Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: Icon components in the BloodHound codebase use BaseSVG which automatically includes VisuallyHidden labels with accessible names from the name prop, so additional aria-label/title attributes are not needed on parent elements.
Applied to files:
packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx
📚 Learning: 2025-08-28T18:42:47.612Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in BloodHound's bh-shared-ui package automatically includes VisuallyHidden labels with accessible names using the pattern "app-icon-{name}", making additional aria-label or title attributes unnecessary on parent elements.
Applied to files:
packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx
🧬 Code graph analysis (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx (1)
packages/javascript/bh-shared-ui/src/components/AppIcon/AppIcon.tsx (1)
AppIcon(28-28)
⏰ 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). (1)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
🔇 Additional comments (11)
packages/javascript/bh-shared-ui/package.json (3)
34-34: Verify downshift v7.0.0 migration is complete and tested.The version bump from downshift
^6.1.7to^7.0.0is a major version change with potential breaking changes to the API. Ensure all downshift-dependent components have been updated to work with v7 and that the changes have been tested thoroughly.From the PR description and provided files, I don't see explicit testing details for this upgrade. Please verify that all usages of downshift throughout the codebase are compatible with v7.
66-66: Accessibility ESLint dependencies added correctly.The
eslint-plugin-jsx-a11y(v6.10.2) and@types/eslint-plugin-jsx-a11y(v6) have been correctly placed indevDependencies. Type definitions belong in dev-time dependencies, and version alignment between the plugin and its types (both major v6) is appropriate.Also applies to: 80-80
66-66: AI summary inconsistency: @types/eslint-plugin-jsx-a11y placement.The AI-generated summary states that
@types/eslint-plugin-jsx-a11ywas added to bothdependenciesanddevDependencies. However, the provided code shows it only indevDependencies(line 66), which is the correct placement. This discrepancy between the summary and actual code is noted.packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.tsx (1)
64-64: Verify removal of getComboboxProps satisfies Axe DevTools.Removing
getComboboxPropsfrom the Downshift hook eliminates ARIA attributes (role="combobox", aria-expanded, aria-haspopup, etc.) typically applied to the wrapper element. Ensure this change satisfies Axe DevTools requirements and that the combobox remains properly announced to screen readers.Based on learnings.
packages/javascript/bh-shared-ui/src/components/SearchResultItem/SearchResultItem.tsx (1)
51-51: Verify thattabIndex={0}is correct for combobox items.Adding
tabIndex={0}places these search result items in the sequential tab order, which typically conflicts with standard combobox keyboard navigation patterns. According to ARIA practices, combobox options should be navigated with arrow keys and not be directly tab-focusable.This pattern may create accessibility issues:
- Users must tab through potentially many search results instead of using arrow keys
- Breaks the expected combobox navigation pattern where focus remains on the input
- May conflict with downshift's built-in focus management (since
getItemPropstypically handles this)Please verify:
- Test keyboard navigation: Can users navigate results with arrow keys? Does Tab move through every result or skip to the next interactive element?
- Confirm this pattern satisfies Axe DevTools requirements and doesn't create a keyboard trap
- Consider whether
tabIndex={-1}or omitting tabIndex (letting downshift handle it) would be more appropriateBased on learnings (Axe DevTools as source of truth)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx (3)
16-16: Verify PopoverAnchor behavior matches requirements.The change from
PopoverTriggertoPopoverAnchoralters how the popover is anchored.PopoverAnchortypically provides positioning without built-in trigger behavior, whereasPopoverTriggerwould handle both positioning and interaction. Since the open state is controlled via theisOpenprop and input changes, this may be intentional.Ensure that this change aligns with the updated
@bloodhoundenterprise/doodleuiAPI and that the popover still opens/closes correctly based on the search input interactions.
102-115: Verify aria-controls override satisfies Axe DevTools.The
aria-controls={undefined}on line 112 explicitly removes thearia-controlsattribute that would typically be provided bygetInputProps(). This attribute normally establishes the relationship between the input and the listbox for screen readers.Based on learnings, similar ARIA attribute overrides (e.g.,
aria-autocomplete={null}in ExploreSearchCombobox) have been used to satisfy Axe DevTools requirements when default attributes cause conflicts. However, please confirm:
- Does Axe DevTools flag the default
aria-controlsattribute as problematic in this context?- Does the
Popovercomponent manage the input-to-menu relationship through a different mechanism?- Have you tested this with screen readers to ensure proper navigation between the input and results list?
The removal of
getComboboxProps()combined witharia-controls={undefined}may break the proper combobox ARIA pattern unless the Popover component compensates for this.Based on learnings
81-81: No issues found with combobox ARIA pattern.The
aria-controls={undefined}on line 112 is intentional. A similar pattern appears elsewhere in the codebase (PrivilegeZones.tsx, line 142-143) with an explicit comment: "aria-controls is optional, and default radix prop breaks accessibility" referencing a known accessibility issue.This pattern aligns with the established approach in BloodHound where property overrides like
aria-controls={undefined}andaria-autocomplete={null}are used to satisfy Axe DevTools requirements, even when they appear to deviate from standard patterns. The menu has no id, making aria-controls unable to establish a meaningful relationship anyway, so the override is appropriate.The combobox structure is properly implemented via
getInputProps()andgetMenuProps()from downshift, which apply the necessary ARIA attributes for combobox functionality.packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx (3)
46-56: LGTM - Simplified downshift integration.Removing
getComboboxPropsfrom the destructured return values aligns with the accessibility refactor pattern in this PR while preserving the necessary functionality through the remaining props.
70-70: LGTM - Consistent with getComboboxProps removal.The wrapper div now uses explicit inline styling instead of spreading
getComboboxProps, which is consistent with the hook changes above.
109-109: No conflicts found; explicit role='listbox' is redundant but harmless.Downshift's getMenuProps() returns ARIA listbox attributes including role="listbox" by default. The explicit
role='listbox'on line 109 is redundant since it duplicates what getMenuProps() already provides. However, this creates no conflict—in JavaScript spread patterns, the later property value simply mirrors the earlier one. This redundancy appears intentional and consistent with the Axe DevTools–driven approach documented in your learnings (similar to thearia-autocomplete={null}override pattern), so no changes are needed.
Comment on lines
112
to
120
| <ListItem | ||
| dense | ||
| className='text-gray-500' | ||
| {...getItemProps({ | ||
| label: 'string', | ||
| item: { | ||
| objectid: '', | ||
| }, | ||
| })}> |
Contributor
There was a problem hiding this comment.
Don't use getItemProps for non-selectable disabled state.
Wrapping the empty/disabled state message with getItemProps is semantically incorrect. This makes the "No results" or loading text appear as a selectable item to assistive technology, which confuses screen readers.
Additionally, label: 'string' (line 116) appears to be a placeholder value rather than meaningful data.
Apply this diff to render the disabled state without item semantics:
- <ListItem
- dense
- className='text-gray-500'
- {...getItemProps({
- label: 'string',
- item: {
- objectid: '',
- },
- })}>
+ <ListItem dense disabled className='text-gray-500'>
<ListItemText primary={disabledText} />
</ListItem>Based on learnings - Axe DevTools should validate that a disabled ListItem properly conveys non-interactive state to screen readers.
📝 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
| <ListItem | |
| dense | |
| className='text-gray-500' | |
| {...getItemProps({ | |
| label: 'string', | |
| item: { | |
| objectid: '', | |
| }, | |
| })}> | |
| <ListItem dense disabled className='text-gray-500'> | |
| <ListItemText primary={disabledText} /> | |
| </ListItem> |
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx
around lines 112-120, the disabled/empty-state ListItem is currently wrapped
with getItemProps and uses a placeholder label ('string'), which gives it
selectable semantics; remove getItemProps from that ListItem and render it as a
plain non-interactive element instead, replace the placeholder label with a
meaningful message (e.g., "No results" or "Loading…"), and add accessibility
attributes such as aria-disabled="true" and role="status" (or
role="presentation" if you just want purely decorative) so screen readers
perceive it as non-selectable and announced correctly. Ensure no item props
(onClick, onMouseDown, etc.) are passed to the element.
Contributor
Author
There was a problem hiding this comment.
I want all of the properties supplied by getItemProps for accessibility purposes. Disabled is not a valid property on ListItem. I will add disabled: true.
Contributor
There was a problem hiding this comment.
@dcairnsspecterops, that makes sense! Passing disabled: true to getItemProps is the correct approach—Downshift will handle the appropriate ARIA attributes (like aria-disabled="true") while preserving all the other accessibility properties you need. This is better than trying to use disabled as a direct prop on the MUI ListItem.
Your solution maintains the proper combobox semantics for assistive technologies while correctly marking the item as non-interactive. 👍
🧠 Learnings used
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.670Z
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.
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.
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.
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.
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.086Z
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.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/utils.tsx already provides accessibility through VisuallyHidden labels, eliminating the need for additional accessible names on wrapper elements.
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.
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx:174-174
Timestamp: 2025-11-06T21:31:03.107Z
Learning: In the BloodHound project, Axe DevTools is the source of truth for accessibility decisions. If a change satisfies Axe DevTools requirements, it should be accepted even if it might appear to violate traditional semantic HTML practices.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: Icon components in the BloodHound codebase use BaseSVG which automatically includes VisuallyHidden labels with accessible names from the name prop, so additional aria-label/title attributes are not needed on parent elements.
packages/javascript/bh-shared-ui/src/components/GraphMenu/GraphMenu.tsx
Outdated
Show resolved
Hide resolved
urangel
approved these changes
Nov 17, 2025
Contributor
urangel
left a comment
urangel
left a comment
There was a problem hiding this comment.
This is fantastic work! Thank you so much for moving the needle on this 🙇 💜
|
|
||
| useLayoutEffect(() => { | ||
| if (cypherEditorRef.current?.cypherEditor) { | ||
| cypherEditorRef.current?.cypherEditor?.codemirror?.contentDOM?.setAttribute('aria-label', 'Cypher Editor'); |
Contributor
Author
There was a problem hiding this comment.
Add a comment here
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx (1)
272-277: Add accessible name to the focusable wrapper.The wrapper div has
role="button"andtabIndex={0}but is missing an accessible name (aria-label or aria-labelledby). Per WCAG 4.1.2, all interactive elements must have accessible names that describe their purpose.Apply this diff to add an appropriate label:
<div role='button' tabIndex={0} onKeyDown={adaptClickHandlerToKeyDown(setFocusOnCypherEditor)} onClick={setFocusOnCypherEditor} + aria-label='Focus Cypher editor' className='flex-1'>
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx (1)
86-90: Add explanatory comment for the useLayoutEffect.As requested in your previous comment, please add a comment explaining why direct DOM manipulation is necessary here (the aria-label prop doesn't reach the correct DOM element per Axe DevTools requirements).
useLayoutEffect(() => { + // Direct DOM manipulation required because the aria-label prop on CypherEditor + // does not properly reach the contentDOM element needed for Axe DevTools compliance if (cypherEditorRef.current?.cypherEditor) { cypherEditorRef.current?.cypherEditor?.codemirror?.contentDOM?.setAttribute('aria-label', 'Cypher Editor'); } }, []);Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx(7 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Cypher/Cypher.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/EntitySelectorsInformation.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectorsList.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectorsList.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/EntitySelectorsInformation.tsx
🧰 Additional context used
🧠 Learnings (18)
📓 Common learnings
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.
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx:174-174
Timestamp: 2025-11-06T21:31:03.144Z
Learning: In the BloodHound project, Axe DevTools is the source of truth for accessibility decisions. If a change satisfies Axe DevTools requirements, it should be accepted even if it might appear to violate traditional semantic HTML practices.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: Icon components in the BloodHound codebase use BaseSVG which automatically includes VisuallyHidden labels with accessible names from the name prop, so additional aria-label/title attributes are not needed on parent elements.
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.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/utils.tsx already provides accessibility through VisuallyHidden labels, eliminating the need for additional accessible names on wrapper elements.
📚 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/PrivilegeZones/Details/SearchBar.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Cypher/Cypher.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx
📚 Learning: 2025-09-08T19:24:33.396Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.test.tsx:17-20
Timestamp: 2025-09-08T19:24:33.396Z
Learning: In BloodHound codebase, prefer importing `render` and `screen` from custom test-utils (e.g., `import { render, screen } from '../../test-utils';`) instead of directly from `testing-library/react`.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.test.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/PrivilegeZones/Cypher/Cypher.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.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/PrivilegeZones/Cypher/Cypher.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx
📚 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/views/PrivilegeZones/Cypher/Cypher.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx
📚 Learning: 2025-08-28T18:42:47.612Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/utils.tsx already provides accessibility through VisuallyHidden labels, eliminating the need for additional accessible names on wrapper elements.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Cypher/Cypher.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx
📚 Learning: 2025-11-06T21:31:03.144Z
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx:174-174
Timestamp: 2025-11-06T21:31:03.144Z
Learning: In the BloodHound project, Axe DevTools is the source of truth for accessibility decisions. If a change satisfies Axe DevTools requirements, it should be accepted even if it might appear to violate traditional semantic HTML practices.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Cypher/Cypher.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx
📚 Learning: 2025-08-28T18:42:47.612Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: Icon components in the BloodHound codebase use BaseSVG which automatically includes VisuallyHidden labels with accessible names from the name prop, so additional aria-label/title attributes are not needed on parent elements.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Cypher/Cypher.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx
📚 Learning: 2025-08-28T18:42:47.612Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in BloodHound's bh-shared-ui package automatically includes VisuallyHidden labels with accessible names using the pattern "app-icon-{name}", making additional aria-label or title attributes unnecessary on parent elements.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Cypher/Cypher.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.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:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Cypher/Cypher.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/PrivilegeZones/Cypher/Cypher.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.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/views/Explore/ExploreSearch/CypherSearch.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/CypherSearch.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/CypherSearch.tsx
📚 Learning: 2025-08-27T15:10:29.731Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: cmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.tsx:206-210
Timestamp: 2025-08-27T15:10:29.731Z
Learning: In the BloodHound codebase, the team prefers to keep JSX elements without keys in the TabPanels component's tabs array, as indicated by the existing ESLint disable comment and explicit decision to not add keys despite Biome linting warnings.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx
📚 Learning: 2025-08-27T15:12:49.129Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/hooks/useSavedQueries.ts:98-105
Timestamp: 2025-08-27T15:12:49.129Z
Learning: In the BloodHound codebase, for the `useDeleteQueryPermissions` hook in `packages/javascript/bh-shared-ui/src/hooks/useSavedQueries.ts`, the team prefers to use broad cache invalidation with `queryClient.invalidateQueries(savedQueryKeys.permissions)` rather than targeted invalidation for specific permission entries.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx
📚 Learning: 2025-08-27T19:15:54.854Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/SavedQueryPermissions.tsx:53-54
Timestamp: 2025-08-27T19:15:54.854Z
Learning: In the BloodHound codebase, the team allows the useQueryPermissions hook to run even with undefined queryId because the API is designed to return errors for empty states, and they prefer to handle these error cases naturally rather than preventing the queries from executing.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx
🧬 Code graph analysis (3)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Cypher/Cypher.tsx (1)
packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (1)
adaptClickHandlerToKeyDown(3-11)
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx (3)
packages/javascript/bh-shared-ui/src/hooks/useSavedQueries.ts (1)
useQueryPermissions(86-92)packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (1)
adaptClickHandlerToKeyDown(3-11)packages/javascript/bh-shared-ui/src/modules.d.ts (1)
CypherEditor(22-24)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx (1)
packages/javascript/bh-shared-ui/src/components/AppIcon/AppIcon.tsx (1)
AppIcon(28-28)
⏰ 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: build-ui
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (8)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Cypher/Cypher.tsx (3)
26-26: LGTM - Import is correct.The import of
adaptClickHandlerToKeyDownis properly added and used to enable keyboard accessibility for the Cypher editor wrapper.
126-131: Keyboard accessibility implementation follows established pattern.The additions of
onKeyDown,tabIndex={0}, and the use ofadaptClickHandlerToKeyDowncorrectly enable keyboard access (Enter/Space keys) to focus the Cypher editor, following the pattern used consistently across the codebase.Note: The
role='textbox'attribute has an ongoing discussion in previous review comments. As established in the learnings, Axe DevTools validation is the source of truth for accessibility decisions in this project.Based on learnings.
134-134: Color contrast enhancement addresses Axe findings.The
brightness-200addition increases color saturation to address color contrast issues identified by the Axe accessibility scanner, as noted in the PR objectives.packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx (1)
296-296: LGTM - Accessibility improvements correctly implemented.The aria-labels are appropriately placed on interactive elements (CypherEditor, buttons, and links) to provide accessible names for screen readers. The previous issue with the aria-label on the nested
<p>tag has been correctly resolved - the aria-label is now on the Button element itself (line 336).Also applies to: 314-314, 328-328, 336-340
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.test.tsx (1)
46-46: LGTM - Test selector updated to match component changes.The test now uses
data-testidinstead of querying by role, providing a stable selector that aligns with thedata-testIdattribute added to the Input component in SearchBar.tsx.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx (3)
16-16: LGTM - Updated to new Popover API.The change from
PopoverTriggertoPopoverAnchoraligns with an updated doodleui API pattern. ThePopoverAnchorserves as a positioning reference while the Input element (withgetInputProps) handles the actual trigger behavior, which is consistent with downshift patterns.Also applies to: 104-115
112-112: LGTM - Test identifier added for stable test selection.The
data-testIdattribute provides a stable selector for testing, aligning with the updated test in SearchBar.test.tsx.
81-81: Verify combobox ARIA pattern implementation in SearchBar.The
getComboboxPropsprop-getter is absent from SearchBar, which means the component is missing critical ARIA attributes required by the WAI-ARIA combobox pattern: role="combobox", aria-expanded, aria-controls, and aria-activedescendant. Downshift v7.0.0 providesgetComboboxPropsfor exactly this purpose.Current implementation uses
PopoverAnchorandPopoverContentfrom doodleui without applyinggetComboboxPropsto a wrapper, which leaves the following unset:
role="combobox"on the containeraria-expanded(reflectingisOpenstate)aria-controls(linking input to menu)- Keyboard event handlers for the combobox pattern
The test file contains only basic functional tests with no accessibility verification. The TODO comment references pending replacement with a doodleui combobox component, suggesting this may be a known gap.
Please confirm:
- Whether
getComboboxPropswas previously used in SearchBar and removed during the downshift upgrade- Whether applying
getComboboxPropsto a wrapper div (wrapping bothPopoverAnchorandPopoverContent) is the intended fix- Whether awaiting the doodleui combobox component is the preferred path forward, or if this should be addressed now
dcairnsspecterops
changed the title
dcairnsspecterops
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.tsx (1)
181-188: Good accessible label on toggle icon; consider dynamic text based on stateAdding the
tipprop here is a solid a11y improvement, giving the toggle icon an accessible name viaIcon. You might optionally refine this by making the label reflect the current state, e.g.:<Icon tip={showSearchWidget ? 'Collapse search widget' : 'Expand search widget'} data-testid='explore_search-container_header_expand-collapse-button' className={classes.icon} onClick={() => setShowSearchWidget((v) => !v)} >This keeps Axe happy while giving screen readers clearer, state-aware text.
packages/javascript/bh-shared-ui/src/.eslintrc.cjs (1)
32-43: Globaljsx-a11y/no-autofocusdisable: confirm intent or consider narrowing scopeTurning
'jsx-a11y/no-autofocus'fully off is reasonable if autofocus patterns are intentional across the app, but it also means new problematic autofocus uses won’t be flagged. Consider either:
- Adding a brief comment here documenting why it’s disabled, or
- Using a narrower configuration (e.g., warning level or file/area-specific disables) if you want some guardrails while preserving known good usages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
cmd/ui/.eslintrc.cjs(2 hunks)cmd/ui/package.json(3 hunks)cmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.tsx(1 hunks)packages/javascript/bh-shared-ui/package.json(3 hunks)packages/javascript/bh-shared-ui/src/.eslintrc.cjs(2 hunks)packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx(3 hunks)packages/javascript/bh-shared-ui/src/components/GraphControls/GraphControls.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/Icon.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.tsx(4 hunks)packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx(7 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx
- cmd/ui/package.json
- packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.test.tsx
- packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.tsx
- packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx
- packages/javascript/bh-shared-ui/src/components/GraphControls/GraphControls.tsx
- cmd/ui/.eslintrc.cjs
- packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx
- packages/javascript/bh-shared-ui/src/components/Icon.tsx
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx:174-174
Timestamp: 2025-11-06T21:31:03.144Z
Learning: In the BloodHound project, Axe DevTools is the source of truth for accessibility decisions. If a change satisfies Axe DevTools requirements, it should be accepted even if it might appear to violate traditional semantic HTML practices.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: Icon components in the BloodHound codebase use BaseSVG which automatically includes VisuallyHidden labels with accessible names from the name prop, so additional aria-label/title attributes are not needed on parent elements.
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.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/utils.tsx already provides accessibility through VisuallyHidden labels, eliminating the need for additional accessible names on wrapper elements.
📚 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/.eslintrc.cjscmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.tsxpackages/javascript/bh-shared-ui/package.json
📚 Learning: 2025-08-28T18:42:47.612Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/utils.tsx already provides accessibility through VisuallyHidden labels, eliminating the need for additional accessible names on wrapper elements.
Applied to files:
packages/javascript/bh-shared-ui/src/.eslintrc.cjs
📚 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:
cmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.tsx
📚 Learning: 2025-08-11T23:53:29.554Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1749
File: packages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.tsx:18-20
Timestamp: 2025-08-11T23:53:29.554Z
Learning: In the bh-shared-ui package, js-file-download is configured as an external dependency in rollup, meaning it's expected to be provided by the consuming environment rather than bundled with the library. This is an intentional architectural decision for dependency management.
Applied to files:
packages/javascript/bh-shared-ui/package.json
⏰ 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). (3)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: run-analysis
🔇 Additional comments (2)
packages/javascript/bh-shared-ui/package.json (1)
34-34: Downshift v7 migration is complete and properly implemented across packages.Both
cmd/uiandbh-shared-uiconsistently use downshift^7.0.0, and the ExploreSearchCombobox component correctly implements the ARIA 1.2 combobox pattern required by downshift v7. The hooks API (useCombobox) and getter functions (getInputProps, getMenuProps, getItemProps) follow v7 conventions, and no v6-specific patterns remain in the codebase. The addition of eslint-plugin-jsx-a11y and @types/eslint-plugin-jsx-a11y to devDependencies appropriately supports the accessibility improvements in this PR.packages/javascript/bh-shared-ui/src/.eslintrc.cjs (1)
20-28: JSX a11y recommended preset fits the accessibility directionAdding
'plugin:jsx-a11y/recommended'toextendsis a solid baseline and aligns with the project’s move to enforce accessibility (and Axe DevTools as the source of truth). Just ensureeslint-plugin-jsx-a11yis installed wherever this config is used so lint runs cleanly. Based on learnings, this is consistent with your Axe-driven a11y strategy.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/javascript/bh-shared-ui/src/components/ColumnHeaders/ColumnHeaders.tsx (1)
60-79: I'll help you verify this review comment. Let me start by examining the file and understanding the current implementation.
<function_calls>
cd /tmp && git clone --branch main https://github.com/SpecterOps/BloodHound.git 2>&1 | head -20
</function_calls>Let me check the file directly:
<function_calls>
find /tmp/BloodHound -name "ColumnHeaders.tsx" -type f
</function_calls><function_calls>
cat /tmp/BloodHound/packages/javascript/bh-shared-ui/src/components/ColumnHeaders/ColumnHeaders.tsx | head -100
</function_calls>Now let me examine lines 60-79 specifically:
<function_calls>
sed -n '50,85p' /tmp/BloodHound/packages/javascript/bh-shared-ui/src/components/ColumnHeaders/ColumnHeaders.tsx
</function_calls><function_calls>
wc -l /tmp/BloodHound/packages/javascript/bh-shared-ui/src/components/ColumnHeaders/ColumnHeaders.tsx
</function_calls>Let me read the entire file to understand it properly:
<function_calls>
cat /tmp/BloodHound/packages/javascript/bh-shared-ui/src/components/ColumnHeaders/ColumnHeaders.tsx
</function_calls>Now let me search for how this component is used and if the disable prop is actually used:
<function_calls>
rg -r "SortableHeader" /tmp/BloodHound/packages/javascript/bh-shared-ui/src --type ts --type tsx -C 3 | head -80
</function_calls>Let me search for uses of the disable prop specifically:
<function_calls>
rg "SortableHeader.*disable" /tmp/BloodHound/packages/javascript/bh-shared-ui/src --type ts --type tsx -A 2 -B 2
</function_calls>Now let me check if there are tests for this component:
<function_calls>
find /tmp/BloodHound/packages/javascript/bh-shared-ui -name "ColumnHeaderstest*" -o -name "ColumnHeaders.spec.*"
</function_calls>Let me search for adaptClickHandlerToKeyDown to understand the utility function:
<function_calls>
rg "adaptClickHandlerToKeyDown" /tmp/BloodHound/packages/javascript/bh-shared-ui --type ts --type tsx -B 5 -A 10
</function_calls>Let me search more broadly:
<function_calls>
rg -l "adaptClickHandlerToKeyDown" /tmp/BloodHound/packages --type ts --type tsx
</function_calls>Let me check for aria-disabled usage in the codebase to understand conventions:
<function_calls>
rg "aria-disabled" /tmp/BloodHound/packages/javascript/bh-shared-ui --type ts --type tsx -C 2
</function_calls>Let me check how keyboard activation is handled elsewhere for comparison:
<function_calls>
rg "role='button'" /tmp/BloodHound/packages/javascript/bh-shared-ui --type ts --type tsx -A 5 -B 2 | head -60
</function_calls>Now let me verify the imports and see if there's any existing accessibility utilities in this component file:
<function_calls>
head -30 /tmp/BloodHound/packages/javascript/bh-shared-ui/src/components/ColumnHeaders/ColumnHeaders.tsx
</function_calls>Let me look at the complete file structure again, focusing on all components:
<function_calls>
cat -n /tmp/BloodHound/packages/javascript/bh-shared-ui/src/components/ColumnHeaders/ColumnHeaders.tsx
</function_calls>Let me search for actual usage of the disable prop being set to true:
<function_calls>
rg "disable\s*=\s*{?\strue\s}?" /tmp/BloodHound/packages/javascript/bh-shared-ui/src --type ts --type tsx -C 3 | grep -A 5 -B 5 "SortableHeader"
</function_calls>Let me search more broadly for where SortableHeader is used:
<function_calls>
rg "SortableHeader" /tmp/BloodHound/packages/javascript/bh-shared-ui/src --type ts --type tsx -B 2 -A 5
</function_calls>Let me check if there's TypeScript interface definition for SortableHeaderProps:
<function_calls>
rg "SortableHeaderProps" /tmp/BloodHound/packages/javascript/bh-shared-ui/src --type ts --type tsx -B 5 -A 10
</function_calls>Let me look at the beginning of the ColumnHeaders file more carefully to see type definitions:
<function_calls>
head -60 /tmp/BloodHound/packages/javascript/bh-shared-ui/src/components/ColumnHeaders/ColumnHeaders.tsx
</function_calls>Let me search the entire repository for disable usage:
<function_calls>
rg "disable\s*=" /tmp/BloodHound/packages/javascript/bh-shared-ui/src/components/ColumnHeaders --type ts --type tsx
</function_calls>Let me check if there are any test files:
<function_calls>
find /tmp/BloodHound/packages/javascript/bh-shared-ui -path "test" -name "ColumnHeader" -o -path "spec" -name "ColumnHeader"
</function_calls>Let me search for all test files related to components:
<function_calls>
find /tmp/BloodHound/packages/javascript/bh-shared-ui -type f ( -name ".test.ts" -o -name ".test.tsx" -o -name ".spec.ts" -o -name ".spec.tsx" ) | head -20
</function_calls>Let me check the learnings to understand context about accessibility patterns in this codebase:
<function_calls>
rg "aria-label" /tmp/BloodHound/packages/javascript/bh-shared-ui/src/components/ColumnHeaders --type ts --type tsx -B 2 -A 2
</function_calls>Let me check common a11y patterns in the codebase for disabled states:
<function_calls>
rg "tabIndex.*-1" /tmp/BloodHound/packages/javascript/bh-shared-ui/src --type ts --type tsx -B 3 -A 1 | head -40
</function_calls>Let me search for all instances of SortableHeader being used in the entire codebase:
<function_calls>
rg "<SortableHeader" /tmp/BloodHound/packages/javascript/bh-shared-ui/src --type ts --type tsx -B 2 -A 8
</function_calls>Let me check if there are any patterns with disable in table-related components:
<function_calls>
find /tmp/BloodHound/packages/javascript/bh-shared-ui/src/components -name "[Tt]able" -type d
</function_calls>Let me check the broader codebase context by searching for common patterns with disabled button roles:
<function_calls>
rg "role=['"]button['"]" /tmp/BloodHound/packages/javascript/bh-shared-ui/src --type ts --type tsx | head -20
</function_calls>Based on my verification, let me now analyze the code and generate the rewritten review comment:
<function_calls>
cat > /tmp/analysis.txt << 'EOF'
VERIFICATION SUMMARY:
FILE EXISTS: ✓
- File: packages/javascript/bh-shared-ui/src/components/ColumnHeaders/ColumnHeaders.tsx
- Component: SortableHeader
- Lines 60-79: Match the provided snippet
CODE MATCHES REVIEW: ✓
- disable prop exists in SortableHeaderProps interface
- Only applies CSS classes: 'pointer-events-none cursor-default'
- onClick handler is not guarded
- onKeyDown handler (via adaptClickHandlerToKeyDown) is not guarded
- tabIndex is hardcoded to 0
- No aria-disabled attribute present
ACCESSIBILITY ISSUES CONFIRMED: ✓
- When disable=true:
- Keyboard events still fire (onKeyDown not guarded)
- Programmatic clicks still work (onClick not guarded)
- tabIndex remains 0 (should be -1 when disabled)
- No aria-disabled to announce state to assistive tech
- Visual disable indication only (pointer-events-none)
USAGE ANALYSIS:
- SortableHeader is used in various table-related components
- The disable prop exists but usage patterns are unclear from grep results
- No clear disable={true} instances found, but prop is defined and available
CODEBASE ACCESSIBILITY CONTEXT:
- Learnings show codebase uses Axe DevTools for a11y compliance
- Pattern shows intentional a11y fixes (e.g., aria-label handling in CypherSearch)
- Suggests this codebase values accessibility compliance
CONCLUSION:
The review comment is ACCURATE regarding:
✓ The disable prop only affects CSS (pointer-events-none)
✓ onClick is not guarded by disable
✓ onKeyDown is not guarded by disable
✓ tabIndex is not conditional
✓ aria-disabled is missingThe suggested fix is APPROPRIATE and follows a11y best practices for disabled interactive elements with role="button".
EOF
cat /tmp/analysis.txt
</function_calls>
disableprop only applies visual CSS; handlers and keyboard activation still function.When
disable={true}:
onClickstill executes on programmatic clicks (not guarded)onKeyDownstill fires for Enter/Space (keyboard activation not blocked despite visible disabling)tabIndexremains 0, keeping the element focusable- Missing
aria-disabledattribute, so assistive technology cannot announce the disabled stateThis creates inconsistency between visual feedback and actual interactivity, plus violates accessibility requirements for interactive elements.
Apply the suggested changes to guard handlers, conditionally set
tabIndex, and addaria-disabled:<div {...rest} role='button' - onClick={onSort} + onClick={disable ? undefined : onSort} tabIndex={0} + tabIndex={disable ? -1 : 0} data-testid='sort-button' - onKeyDown={adaptClickHandlerToKeyDown(onSort)} + onKeyDown={disable ? undefined : adaptClickHandlerToKeyDown(onSort)} aria-label={`Sort by ${title}`} + aria-disabled={disable || undefined} className={cn({ 'pointer-events-none cursor-default': disable }, containerClass)}>This ensures disabled headers are not keyboard-activatable, not focusable, and properly announced to assistive technologies.
🧹 Nitpick comments (2)
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoHeader.test.tsx (1)
88-95: Good switch to accessible names; consider consistent case‑insensitive matching for tooltipsUsing descriptive labels (
"Clear selected item","Collapse All") and role queries is a solid a11y‑aligned improvement, and the hover/tooltip expectations look correct. To make the test a bit more robust to future casing tweaks, you could mirror the case‑insensitive pattern you already use for"collapse all":- expect(await screen.findByRole('tooltip', { name: /Clear selected item/ })).toBeInTheDocument(); + expect(await screen.findByRole('tooltip', { name: /clear selected item/i })).toBeInTheDocument();packages/javascript/bh-shared-ui/src/components/ColumnHeaders/ColumnHeaders.tsx (1)
80-83: InnerButtoninert/role='none'pattern is unusual but acceptable; consider a small compatibility check.Making the inner
Buttoninert androle='none'so the outer div+role='button' is the only interactive control is a reasonable way to avoid nested interactive elements while keeping the visual styling. Given that Axe DevTools is your source of truth, this is fine as long as it passes Axe.If you run into browser/AT compatibility issues, you might also:
- Add
tabIndex={-1}on theButtonas a belt-and-suspenders fallback.- Or gate the
inertapplication behind a feature check (e.g., only set it whenHTMLElement.prototype.hasOwnProperty('inert')in a layout effect).Not required if your current Axe checks and target browsers are happy, but worth considering if you see odd focus or announcement behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/javascript/bh-shared-ui/src/components/ColumnHeaders/ColumnHeaders.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoHeader.test.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/Explore/EdgeInfo/EdgeInfoHeader.test.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx:174-174
Timestamp: 2025-11-06T21:31:03.144Z
Learning: In the BloodHound project, Axe DevTools is the source of truth for accessibility decisions. If a change satisfies Axe DevTools requirements, it should be accepted even if it might appear to violate traditional semantic HTML practices.
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.
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.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/utils.tsx already provides accessibility through VisuallyHidden labels, eliminating the need for additional accessible names on wrapper elements.
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.
📚 Learning: 2025-08-28T18:42:47.612Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/utils.tsx already provides accessibility through VisuallyHidden labels, eliminating the need for additional accessible names on wrapper elements.
Applied to files:
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoHeader.test.tsxpackages/javascript/bh-shared-ui/src/components/ColumnHeaders/ColumnHeaders.tsx
📚 Learning: 2025-09-08T19:24:33.396Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.test.tsx:17-20
Timestamp: 2025-09-08T19:24:33.396Z
Learning: In BloodHound codebase, prefer importing `render` and `screen` from custom test-utils (e.g., `import { render, screen } from '../../test-utils';`) instead of directly from `testing-library/react`.
Applied to files:
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoHeader.test.tsxpackages/javascript/bh-shared-ui/src/views/Explore/EdgeInfo/EdgeInfoHeader.test.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/EdgeInfo/EdgeInfoHeader.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsxpackages/javascript/bh-shared-ui/src/components/ColumnHeaders/ColumnHeaders.tsx
📚 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/views/PrivilegeZones/Details/Details.test.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/PrivilegeZones/Details/Details.test.tsxpackages/javascript/bh-shared-ui/src/components/ColumnHeaders/ColumnHeaders.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/PrivilegeZones/Details/Details.test.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/PrivilegeZones/Details/Details.test.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/PrivilegeZones/Details/Details.test.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:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.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/views/PrivilegeZones/Details/Details.test.tsx
📚 Learning: 2025-11-06T21:31:03.144Z
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx:174-174
Timestamp: 2025-11-06T21:31:03.144Z
Learning: In the BloodHound project, Axe DevTools is the source of truth for accessibility decisions. If a change satisfies Axe DevTools requirements, it should be accepted even if it might appear to violate traditional semantic HTML practices.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx
📚 Learning: 2025-08-27T15:10:29.731Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: cmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.tsx:206-210
Timestamp: 2025-08-27T15:10:29.731Z
Learning: In the BloodHound codebase, the team prefers to keep JSX elements without keys in the TabPanels component's tabs array, as indicated by the existing ESLint disable comment and explicit decision to not add keys despite Biome linting warnings.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx
📚 Learning: 2025-08-27T21:15:32.207Z
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.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx
🧬 Code graph analysis (1)
packages/javascript/bh-shared-ui/src/components/ColumnHeaders/ColumnHeaders.tsx (2)
packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (1)
adaptClickHandlerToKeyDown(3-11)packages/javascript/bh-shared-ui/src/utils/theme.ts (1)
cn(41-43)
⏰ 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). (3)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: run-analysis
🔇 Additional comments (7)
packages/javascript/bh-shared-ui/src/views/Explore/EdgeInfo/EdgeInfoHeader.test.tsx (1)
69-69: LGTM! Test updates correctly reflect accessibility improvements.The test assertions have been properly updated to use descriptive accessible names ('Clear selected item', 'Collapse All') instead of icon identifiers ('xmark', 'angles-up'). These changes align with the PR's accessibility goals and ensure tests locate elements by their accessible names.
Based on learnings, these changes satisfy Axe DevTools requirements for accessible naming.
Also applies to: 73-73, 83-83, 92-92
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoHeader.test.tsx (1)
102-116: Click behavior tests correctly track new button labelsUpdating the click tests to locate controls by the new accessible names (
"Collapse All"and"Clear selected item") keeps the behavior assertions intact while aligning with the improved UI copy. The expectations on URL search params andmockClearSelectedItemremain valid and focused on behavior rather than implementation details.packages/javascript/bh-shared-ui/src/components/ColumnHeaders/ColumnHeaders.tsx (1)
28-45: Importing shared utilities here looks correct and consistent.Using
adaptClickHandlerToKeyDownandcnfrom the shared utils module keeps keyboard behavior and class merging consistent with the rest of the app. Both imports are used correctly in this file.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx (4)
83-86: Test queries updated appropriately for test-id based selection.The replacement of role-based queries with specific test-id queries (
selector-row,member-row) aligns with the accessibility changes and maintains async patterns consistently.
138-138: Good improvement to query specificity.Scoping the
findByTextquery withwithin(rules)improves test reliability by ensuring the selector is found within the correct container.
115-115: I'll help you verify the test-id inconsistency at line 115. Let me start by examining the test file and related component implementations.
<function_calls>
#!/bin/bashFirst, locate and examine the Details.test.tsx file
fd "Details.test.tsx" --type f
</function_calls>Now let me view the relevant sections of the test file:
<function_calls>
#!/bin/bashView the test file around lines 86 and 115 to understand context
if fd "Details.test.tsx" --type f | head -1 | xargs cat -n | grep -A 5 -B 5 "^.*86|^.*115" > /dev/null; then
fd "Details.test.tsx" --type f | head -1 | xargs cat -n | sed -n '80,120p'
fi
</function_calls><function_calls>
#!/bin/bashGet file size and content
FILE=$(fd "Details.test.tsx" --type f | head -1)
wc -l "$FILE"
</function_calls><function_calls>
#!/bin/bashView lines 80-120 from the Details.test.tsx file
FILE=$(fd "Details.test.tsx" --type f | head -1)
sed -n '80,120p' "$FILE" | cat -n
</function_calls><function_calls>
#!/bin/bashSearch for the Details component to understand the UI structure
fd "Details.tsx" --type f | grep -v test | head -1
</function_calls><function_calls>
#!/bin/bashSearch for 'sort-button' and 'member-row' test-id definitions in the codebase
echo "=== Searching for 'sort-button' test-id ==="
rg -n "sort-button" --type=tsx --type=ts -C 2echo -e "\n=== Searching for 'member-row' test-id ==="
rg -n "member-row" --type=tsx --type=ts -C 2
</function_calls>Please provide the shell script results so I can analyze the test file content and verify the test-id usage patterns.
158-167: Based on my investigation of the codebase, I can now provide a definitive verification:The
sort-buttonelements are correctly structured as divs with exactly one child.The
SortableHeadercomponent (inpackages/javascript/bh-shared-ui/src/components/ColumnHeaders/ColumnHeaders.tsx) renders:
- A
<div data-testid='sort-button' role='button'>that contains- Exactly one
<Button>child elementThe test assertion
expect(li.childNodes).toHaveLength(1)is correct — each sort-button div has exactly one direct child (the Button component). This is a valid structural sanity check.However, these assertions (lines 158–167) are structural verification checks, not deselection behavior tests. The actual deselection verification happens later (lines 169–175) when the test clicks a different zone and checks for the active tag update.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.tsx (1)
119-140: Consider simplifying the no-results message rendering.The current approach creates a synthetic
FlatNodeobject with all properties set to empty/default values just to render a "No result found" message throughgetItemProps. This introduces maintenance overhead—if theFlatNodetype evolves, this mock object must be kept in sync.Consider a simpler approach that renders the message outside the combobox item pattern:
- {items.length === 0 && inputValue && ( - <li - className='text-sm opacity-70' - {...getItemProps({ - disabled: true, - 'aria-disabled': true, - label: NO_RESULTS_TEXT, - item: { - id: '', - label: '', - kind: '', - objectId: '', - lastSeen: '', - isTierZero: false, - isOwnedObject: false, - descendent_count: null, - properties: {}, - }, - })}> - {NO_RESULTS_TEXT} - </li> - )} + {items.length === 0 && inputValue && ( + <li className='text-sm opacity-70' role='status' aria-live='polite'> + {NO_RESULTS_TEXT} + </li> + )}However, if the current approach is required to satisfy Axe DevTools requirements, please disregard this suggestion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx:174-174
Timestamp: 2025-11-06T21:31:03.144Z
Learning: In the BloodHound project, Axe DevTools is the source of truth for accessibility decisions. If a change satisfies Axe DevTools requirements, it should be accepted even if it might appear to violate traditional semantic HTML practices.
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.
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.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/utils.tsx already provides accessibility through VisuallyHidden labels, eliminating the need for additional accessible names on wrapper elements.
📚 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/components/SearchCurrentNodes/SearchCurrentNodes.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:
packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.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/components/SearchCurrentNodes/SearchCurrentNodes.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/components/SearchCurrentNodes/SearchCurrentNodes.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/SearchCurrentNodes/SearchCurrentNodes.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/SearchCurrentNodes/SearchCurrentNodes.tsx
🧬 Code graph analysis (1)
packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.tsx (1)
packages/javascript/bh-shared-ui/src/utils/theme.ts (1)
cn(41-43)
⏰ 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). (3)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (3)
packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.tsx (3)
17-17: LGTM: MUI TextField improves accessibility.Replacing the internal Input component with MUI TextField provides better built-in accessibility support.
143-149: LGTM: TextField properly integrates with downshift.The TextField correctly receives
inputPropsfromgetInputProps(), ensuring necessary ARIA attributes are applied. The className merging with thecnutility works well with MUI TextField.
65-65: Based on my verification, I can now provide the rewritten review comment:
Combobox ARIA pattern correctly implements downshift v7 specifications.
The removal of
getComboboxPropsis the correct approach for downshift v7's migration from ARIA 1.1 to ARIA 1.2. The component properly uses the v7 API:
- TextField receives
getInputProps()(line 147), providing combobox input attributes- Menu
<ul>receivesgetMenuProps()(lines 104-109), providing listbox attributes- Menu items receive
getItemProps()(lines 122-137), providing option attributesThis structure adheres to downshift v7's required ARIA 1.2 combobox pattern and is consistent with other migrated components in the codebase like ExploreSearchCombobox.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/javascript/bh-shared-ui/src/components/FileDrop/FileDrop.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx:174-174
Timestamp: 2025-11-06T21:31:03.144Z
Learning: In the BloodHound project, Axe DevTools is the source of truth for accessibility decisions. If a change satisfies Axe DevTools requirements, it should be accepted even if it might appear to violate traditional semantic HTML practices.
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.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/utils.tsx already provides accessibility through VisuallyHidden labels, eliminating the need for additional accessible names on wrapper elements.
📚 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/components/FileDrop/FileDrop.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:
packages/javascript/bh-shared-ui/src/components/FileDrop/FileDrop.tsx
🧬 Code graph analysis (1)
packages/javascript/bh-shared-ui/src/components/FileDrop/FileDrop.tsx (1)
packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (1)
adaptClickHandlerToKeyDown(3-11)
⏰ 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: run-tests
- GitHub Check: build-ui
🔇 Additional comments (2)
packages/javascript/bh-shared-ui/src/components/FileDrop/FileDrop.tsx (2)
20-20: LGTM!The import of
adaptClickHandlerToKeyDownis correctly added and used for keyboard accessibility.
91-91: Based on my verification, I can now provide the final assessment:Type compatibility is valid and not an error.
The
handleClickfunction with signature() => voidis assignable to theKeyboardEventHandler<HTMLElement>parameter expected byadaptClickHandlerToKeyDown. This is valid TypeScript behavior due to function parameter covariance rules—a function that ignores parameters is compatible with a function type expecting those parameters. The codebase uses TypeScript 5.1.6 in strict mode withtsc --noEmitas part of the build pipeline, and the code compiles successfully without type errors.
Comment on lines
81
to
92
| <div | ||
| role='button' | ||
| tabIndex={0} | ||
| className='absolute size-full' | ||
| onClick={handleClick} | ||
| onDragEnter={handleDragEnter} | ||
| onDragLeave={handleDragLeave} | ||
| onDragOver={handleDragOver} | ||
| onMouseEnter={handleMouseEnter} | ||
| onMouseLeave={handleMouseLeave} | ||
| onKeyDown={adaptClickHandlerToKeyDown(handleClick)} | ||
| onDrop={handleDrop}></div> |
Contributor
There was a problem hiding this comment.
Add aria-label and respect disabled state for keyboard accessibility.
The overlay div has two accessibility gaps:
-
Missing accessible name: The
role='button'requires an accessible name for screen readers. The visual text is a sibling, not associated with this button. -
Disabled state not enforced: When
disabled={true}, the button remains keyboard-focusable and interactive. ThetabIndexshould be conditional, and handlers should check the disabled prop to prevent activation.
Apply this diff:
+ const effectiveTabIndex = disabled ? -1 : 0;
<div
role='button'
- tabIndex={0}
+ tabIndex={effectiveTabIndex}
+ aria-label='Upload JSON or zip files'
+ aria-disabled={disabled}
className='absolute size-full'
- onClick={handleClick}
+ onClick={disabled ? undefined : handleClick}
onDragEnter={handleDragEnter}
onDragLeave={handleDragLeave}
onDragOver={handleDragOver}
onMouseEnter={handleMouseEnter}
onMouseLeave={handleMouseLeave}
- onKeyDown={adaptClickHandlerToKeyDown(handleClick)}
+ onKeyDown={disabled ? undefined : adaptClickHandlerToKeyDown(handleClick)}
onDrop={handleDrop}></div>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/javascript/bh-shared-ui/src/components/FileDrop/FileDrop.tsx around
lines 81 to 92, the overlay div acting as a button lacks an accessible name and
does not respect the disabled prop; add an accessible name (aria-label or
aria-labelledby) that matches the visible control text and make focusability
conditional by setting tabIndex to -1 when disabled and 0 otherwise, and update
all interactive handlers (onClick, onKeyDown via adaptClickHandlerToKeyDown,
onDrop, onDragEnter/Leave/Over, onMouseEnter/Leave) to short-circuit when
disabled so no action occurs; ensure aria-disabled is set to true when disabled
for assistive tech.
elikmiller
approved these changes
Dec 4, 2025
dcairnsspecterops
changed the title
dcairnsspecterops
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/EntityRulesInformation.tsx (2)
97-101: Consider explicitly settingtype="button"on the ellipsis triggerNice job giving the icon-only trigger a clear
aria-label. To future‑proof this in case it’s ever placed inside a<form>, consider explicitly setting the button type:- <button - onClick={() => handleMenuClick(index)} - aria-label='Show selector options'> + <button + type='button' + onClick={() => handleMenuClick(index)} + aria-label='Show selector options'>This avoids accidental form submission behavior.
108-116: Keyboard support for View/Edit items is good; consider leveraging native button semantics and the event objectThe added
role="button",tabIndex={0}, andonKeyDown={adaptClickHandlerToKeyDown(...)}make the View/Edit actions keyboard-accessible, which is great.Two non‑blocking refinements to consider:
- Prefer real buttons over div+role when possible:
- <div - role='button' - tabIndex={0} - className='cursor-pointer p-2 hover:bg-neutral-4' - onClick={() => { - handleViewClick(rule.id); - }} - onKeyDown={adaptClickHandlerToKeyDown(() => { - handleViewClick(rule.id); - })}> + <button + type='button' + className='cursor-pointer p-2 hover:bg-neutral-4' + onClick={() => { + handleViewClick(rule.id); + }}> View - </div> + </button>…and similarly for the Edit action. Native
<button>elements bring built‑in semantics, focus behavior, and Enter/Space handling without needingrole,tabIndex, or a custom key adapter—provided this still passes Axe in your setup.
- If you keep the
div role="button"pattern withadaptClickHandlerToKeyDown, consider accepting the event and mirroring native button behavior (e.g., preventing default on Space):- onKeyDown={adaptClickHandlerToKeyDown(() => { - handleViewClick(rule.id); - })}> + onKeyDown={adaptClickHandlerToKeyDown((event) => { + event.preventDefault(); + handleViewClick(rule.id); + })}>Same idea for the Edit handler. This keeps the helper aligned with typical button keyboard semantics while preserving the Axe-driven approach. Based on learnings, Axe DevTools is the source of truth here, so treat this as optional as long as scans stay clean.
Also applies to: 121-126
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Cypher/Cypher.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/EntityRulesInformation.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/RulesList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/Users/UserActionsMenu.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/Users/Users.test.tsx(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/RulesList.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx
- packages/javascript/bh-shared-ui/src/views/Users/UserActionsMenu.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx
- packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Cypher/Cypher.tsx
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
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.
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx:174-174
Timestamp: 2025-11-06T21:31:03.144Z
Learning: In the BloodHound project, Axe DevTools is the source of truth for accessibility decisions. If a change satisfies Axe DevTools requirements, it should be accepted even if it might appear to violate traditional semantic HTML practices.
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.
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.
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/utils.tsx already provides accessibility through VisuallyHidden labels, eliminating the need for additional accessible names on wrapper elements.
📚 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:
packages/javascript/bh-shared-ui/src/views/Users/Users.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx
📚 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/views/PrivilegeZones/Details/Details.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/EntityRulesInformation.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/PrivilegeZones/Details/Details.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/EntityRulesInformation.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/PrivilegeZones/Details/Details.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/EntityRulesInformation.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/PrivilegeZones/Details/Details.test.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/PrivilegeZones/Details/Details.test.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/PrivilegeZones/Details/Details.test.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/views/PrivilegeZones/Details/Details.test.tsx
📚 Learning: 2025-11-06T21:31:03.144Z
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx:174-174
Timestamp: 2025-11-06T21:31:03.144Z
Learning: In the BloodHound project, Axe DevTools is the source of truth for accessibility decisions. If a change satisfies Axe DevTools requirements, it should be accepted even if it might appear to violate traditional semantic HTML practices.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx
📚 Learning: 2025-08-27T15:10:29.731Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: cmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.tsx:206-210
Timestamp: 2025-08-27T15:10:29.731Z
Learning: In the BloodHound codebase, the team prefers to keep JSX elements without keys in the TabPanels component's tabs array, as indicated by the existing ESLint disable comment and explicit decision to not add keys despite Biome linting warnings.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx
📚 Learning: 2025-09-08T20:04:09.070Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/QuerySearchFilter.tsx:110-116
Timestamp: 2025-09-08T20:04:09.070Z
Learning: In BloodHound's TypeScript codebase, jvacca-specterops considers runtime guards for type assertions unnecessary when the UI state already prevents the problematic scenario (e.g., disabled buttons when required data is unavailable), preferring to rely on the existing disabled state logic rather than adding additional runtime checks.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx
🧬 Code graph analysis (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/EntityRulesInformation.tsx (1)
packages/javascript/bh-shared-ui/src/utils/adaptClickHandlerToKeyDown.tsx (1)
adaptClickHandlerToKeyDown(3-11)
⏰ 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 (5)
packages/javascript/bh-shared-ui/src/views/Users/Users.test.tsx (1)
54-54: LGTM! Test selectors correctly updated to match accessibility improvements.The test selectors have been consistently updated across all five test cases to use the new accessible name "Show user actions". This change:
- Makes the tests more specific and reliable by explicitly targeting the correct button
- Validates that the accessibility improvements are properly implemented
- Improves test maintainability by using semantic selectors instead of positional queries
Also applies to: 90-90, 100-100, 109-109, 134-134
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx (3)
82-103: AsyncfindAllByTestId('member-row')usage here looks solidSwitching from a role-based selector to
await within(objects).findAllByTestId('member-row')keeps this test aligned with the updated DOM structure and should be more resilient as accessibility roles change. The subsequent assertions onobjectsListItemsstill match the original intent.
114-165: Confirmsort-buttonis the right target for “objects list items” in these testsBoth tests now use
await within(objects).findAllByTestId('sort-button')and then reason aboutobjectsListItems(including checkingchildNodeslength in the later test). Ifsort-buttonrefers to a control element (e.g., a header sort toggle) rather than the actual object rows, these assertions may no longer be verifying the intended “no object selected” behavior.Please double‑check the rendered markup to ensure that:
- The elements with
data-testid="sort-button"are indeed the items whose selection state you care about, or- If not, consider introducing/using a more semantically named test-id for the object rows and updating these assertions accordingly.
166-173: Zones list presence check using static-order test-id is consistent with new markupUsing
await within(zones).findAllByTestId('privilege-zones_details_zones-list_static-order')as a readiness check before interacting with the “Tier-2” zone looks appropriate and keeps the test tied to the new data-testid-based structure rather than listitem roles. The rest of the selection assertion remains unchanged and accurate.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/EntityRulesInformation.tsx (1)
22-22: ImportingadaptClickHandlerToKeyDownvia the utils barrel looks goodUsing the utils barrel here is consistent and keeps the import surface tidy. No issues.
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
This PR adds accessibility support by 1) addressing all issues on all pages discovered by the axe accessibility scanner devtools, 2) adds a linter to enforce good accessibility practices in the future:
eslint-plugin-jsx-a11y.Screenshot below detail the Before (right) and After (left) state using Axe devtools for accessibility scanning.
Stubborn issues:
Note: NPM audit calls my package changes safe, save for this:
Accessibility issues After // Before
Major changes addressed
Motivation and Context
Resolves BED-5690
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
Chores
✏️ Tip: You can customize this high-level summary in your review settings.