BED-6021 Explore Graph Data Table - Manage Columns #1602
Conversation
…--table-view-setup
…--table-view-setup
…oodHound into BED-3538--table-view-setup
…oodHound into BED-3538--table-view-setup
…--table-view-setup
…--table-view-setup
| @@ -66,5 +76,6 @@ export const useExploreGraph = (includeProperties?: boolean) => { | |||
| autoHideDuration: SNACKBAR_DURATION_LONG, | |||
| }); | |||
| }, | |||
| enabled, | |||
There was a problem hiding this comment.
it seems that adding this enabled field at the end of the properties will override the enabled defined within each queryConfig, which results in the query trying to execute without a queryFn yet.
Perhaps we need to move the enabled field up above the queryConfig, or pass it to getQueryConfig?
| export const useExploreGraph = ({ | ||
| includeProperties, | ||
| enabled, | ||
| }: UseExploreGraphParams = DEFAULT_USE_EXPLORE_GRAPH_PARAMS) => { |
There was a problem hiding this comment.
If i pass includeProperties: true, will enabled then be false because the fallback object isnt triggered? If so, is that intended?
.../javascript/bh-shared-ui/src/hooks/useExploreTableAutoDisplay/useExploreTableAutoDisplay.tsx
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.tsx
Outdated
Show resolved
Hide resolved
| const mungedData = useMemo( | ||
| () => (data && Object.keys(data).map((id) => ({ ...data?.[id]?.data, id }))) || [], | ||
| () => | ||
| // TODO: remove id and just use objectid for onRowClick/getRowId? |
There was a problem hiding this comment.
this should work because the selectedItem param works with a db id or objectid!
packages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.tsx
Outdated
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.tsx
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/javascript/bh-shared-ui/src/components/ExploreTable/ManageColumnsComboBox/ManageColumnsComboBox.tsx (2)
94-103: Simplify the handleSelectAll logic.The function calls
handleResetDefault()in both branches, making the conditional logic redundant as mentioned in previous reviews.Apply this diff to streamline the logic:
const handleSelectAll = () => { + handleResetDefault(); if (shouldSelectAll) { - handleResetDefault(); setSelectedColumns([...allColumns]); onChange([...allColumns]); - } else { - handleResetDefault(); } - return shouldSelectAll; };
148-160: Fix potential React key warnings in mapped items.The
.map()callback doesn't return anything for pinned items (line 159), which could cause React warnings about undefined array elements.Apply this diff to fix the mapping issue:
- ...selectedColumns.map((column, index) => { - if (!column?.isPinned) { - return ( - <ManageColumnsListItem - isSelected - key={`${column?.id}-${index}`} - item={column} - onClick={removeSelectedItem} - itemProps={getItemProps({ item: column, index })} - /> - ); - } - }), + ...selectedColumns + .filter(column => !column?.isPinned) + .map((column, index) => ( + <ManageColumnsListItem + isSelected + key={`${column?.id}-${index}`} + item={column} + onClick={removeSelectedItem} + itemProps={getItemProps({ item: column, index })} + /> + )),
🧹 Nitpick comments (3)
packages/javascript/bh-shared-ui/src/components/ExploreTable/ManageColumnsComboBox/ManageColumnsComboBox.tsx (3)
53-62: Optimize useMultipleSelection initial state.The
initialSelectedItemson line 54 recomputes the same value asinitialColumns(line 29-32), causing unnecessary work. Use the memoized value instead.Apply this diff to use the memoized value:
const { getDropdownProps, removeSelectedItem, addSelectedItem } = useMultipleSelection({ - initialSelectedItems: allColumns.filter((item) => selectedColumnsProp[item.id]), + initialSelectedItems: initialColumns, selectedItems: selectedColumns, onStateChange({ selectedItems: newSelectedColumns, type }) { if (type !== useMultipleSelection.stateChangeTypes.DropdownKeyDownBackspace) { setSelectedColumns(newSelectedColumns || []); onChange(newSelectedColumns || []); } }, });
72-77: Improve state consistency in ItemClick handler.When adding a new item, the component updates
selectedColumnsbut doesn't callonChangeuntil theonStateChangecallback, creating a potential timing issue where local state and parent state could be briefly out of sync.Consider removing the direct state update here since
onStateChangewill handle it:case useCombobox.stateChangeTypes.ItemClick: if (newSelectedItem) { - setSelectedColumns([...selectedColumns, newSelectedItem]); + addSelectedItem(newSelectedItem); setInputValue(''); } break;
117-118: Consider extracting dropdown styling to constants.The hardcoded positioning (
top-3) and width (w-[400px]) values should be extracted to constants or made configurable through props for better maintainability.Consider extracting these values:
+const DROPDOWN_POSITIONING = 'top-3'; +const DROPDOWN_WIDTH = 'w-[400px]'; + - <div className={`${isOpen ? '' : 'hidden'} absolute z-20 top-3`} ref={ref}> - <div className='w-[400px] shadow-md border-1 bg-white' {...getComboboxProps()}> + <div className={`${isOpen ? '' : 'hidden'} absolute z-20 ${DROPDOWN_POSITIONING}`} ref={ref}> + <div className={`${DROPDOWN_WIDTH} shadow-md border-1 bg-white`} {...getComboboxProps()}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/javascript/bh-shared-ui/src/components/ExploreTable/ManageColumnsComboBox/ManageColumnsComboBox.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/ExploreTable/ManageColumnsComboBox/ManageColumnsListItem.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/javascript/bh-shared-ui/src/components/ExploreTable/ManageColumnsComboBox/ManageColumnsListItem.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/javascript/bh-shared-ui/src/components/ExploreTable/ManageColumnsComboBox/ManageColumnsComboBox.tsx (2)
packages/javascript/bh-shared-ui/src/components/ExploreTable/index.ts (1)
ManageColumnsComboBoxOption(18-18)packages/javascript/bh-shared-ui/src/components/ExploreTable/explore-table-utils.ts (1)
makeStoreMapFromColumnOptions(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 (2)
packages/javascript/bh-shared-ui/src/components/ExploreTable/ManageColumnsComboBox/ManageColumnsComboBox.tsx (2)
1-16: Well-structured imports and type definitions.The imports are appropriate for the component functionality, and the
ManageColumnsComboBoxOptiontype is well-defined with clear properties. The component props interface is clean and type-safe.
29-40: Good state synchronization implementation.The
useEffecton lines 33-35 properly synchronizes the localselectedColumnsstate with the prop changes, addressing the previous concern about desynchronization. The memoizedinitialColumnsensures efficient re-computation only when dependencies change.
| export const useExploreGraph = ({ | ||
| includeProperties = DEFAULT_USE_EXPLORE_GRAPH_PARAMS.includeProperties, | ||
| enabled = DEFAULT_USE_EXPLORE_GRAPH_PARAMS.enabled, | ||
| }: UseExploreGraphParams = DEFAULT_USE_EXPLORE_GRAPH_PARAMS) => { |
There was a problem hiding this comment.
Constants are typically a good idea, but in this case, I'd argue for simplicity.
export const useExploreGraph = ({ includeProperties = false, enabled = true }: UseExploreGraphParams) => {There was a problem hiding this comment.
The issue here is that the parameter needs a default, since this existing hook is already called throughout the app with no parameters. The alternative is to find every use of this hook and pass it an empty object(?), or check for the existence of the object inside the function, which ended up feeling messier than this (since now the object needs a name and we need to pull the vars inside the call)
| includeProperties?: boolean; | ||
| enabled?: boolean; | ||
| }; | ||
|
|
||
| export function exploreGraphQueryFactory(paramOptions: Partial<ExploreQueryParams>): ExploreGraphQuery { |
There was a problem hiding this comment.
It looks like cypherSearchGraphQuery unfortunately does not completely align with the others queries. It has an extra param of includeProperties which means its type is not actually ExploreGraphQuery. It looks like there is no easy way around that. But we can make a small improvement to the typing to reflect what's happening.
export function exploreGraphQueryFactory(paramOptions: Partial<ExploreQueryParams>): ExploreGraphQuery | CypherExploreGraphQuery {With that, we could make a second small improvement to not need the type assertion on line 68
const queryConfig =
params?.searchType === 'cypher'
? query.getQueryConfig(params, includeProperties)
: query.getQueryConfig(params);There was a problem hiding this comment.
That's much nicer, thanks.
| if (typeof value === 'boolean') { | ||
| return value ? ( | ||
| <div className='h-full w-full flex justify-center items-center text-center'> | ||
| <FontAwesomeIcon icon={faCheck} color='green' className='scale-125' />{' '} | ||
| </div> | ||
| ) : ( | ||
| <div className='h-full w-full flex justify-center items-center text-center'> | ||
| <FontAwesomeIcon icon={faCancel} color='lightgray' className='scale-125' />{' '} | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
You can use ternaries to keep this a little DRYer.
| if (typeof value === 'boolean') { | |
| return value ? ( | |
| <div className='h-full w-full flex justify-center items-center text-center'> | |
| <FontAwesomeIcon icon={faCheck} color='green' className='scale-125' />{' '} | |
| </div> | |
| ) : ( | |
| <div className='h-full w-full flex justify-center items-center text-center'> | |
| <FontAwesomeIcon icon={faCancel} color='lightgray' className='scale-125' />{' '} | |
| </div> | |
| ); | |
| } | |
| if (typeof value === 'boolean') { | |
| return ( | |
| <div className='h-full w-full flex justify-center items-center text-center'> | |
| <FontAwesomeIcon | |
| icon={value ? faCheck : faCancel} | |
| color={value ? 'green' : 'lightgray'} | |
| className='scale-125' | |
| />{' '} | |
| </div> | |
| ); | |
| } |
There was a problem hiding this comment.
Yep, an obviously good idea which, I feel like if I were the reviewer I would have caught, but somehow slipped my awareness in my own code. Thanks much.
| const filterKeys: (keyof GraphNode)[] = ['displayname', 'objectid']; | ||
| const filterTargets = filterKeys.map((filterKey) => { | ||
| const stringyValue = String(potentialRow?.[filterKey]); | ||
|
|
||
| return stringyValue?.toLowerCase(); | ||
| }); |
There was a problem hiding this comment.
Two small suggestions here. The definition on line 94 can be moved outside of the component as a module constant. This way, it's not recreated for every row. If you use the ordering on line 141, you could use the const there as well, since the order would matter in that case: ...['objectid', 'displayname'].map(makeColumnDef),.
And lines 95-99 can be compressed into a 1 liner. String() will always return a string value no matter what you throw at it, so no need for the optional chaining operator (?.). The optional chains are another place where the small performance overhead adds up, so ideally, they should only be used when really needed.
const filterTargets = filterKeys.map((filterKey) => String(item[filterKey]).toLowerCase());| [nonRequiredColumnDefinitions, selectedColumns] | ||
| ); | ||
|
|
||
| const requiredColumnDefinitions = useMemo( |
There was a problem hiding this comment.
Since all the items in this value are static, instead of the memo, you can just move this to a constant outside of the component within the module scope. Then you could remove the value from the deps below to reduce some renders.
| const tableHeaderProps: DataTableProps['TableHeaderProps'] = useMemo( | ||
| () => ({ | ||
| className: 'sticky top-0 z-10', | ||
| }), | ||
| [] | ||
| ); | ||
|
|
||
| const tableHeadProps: DataTableProps['TableHeadProps'] = useMemo( | ||
| () => ({ | ||
| className: 'pr-4', | ||
| }), | ||
| [] | ||
| ); |
There was a problem hiding this comment.
I'm assuming these were memoized because they are object values that were causing the data table to re-render on every ExploreTable render. If so, this is another scenario you could move the values to module-scope constants above the component. useMemo isn't free, so in cases where you aren't using component-scope members, constants are a great go-to over memos.
There was a problem hiding this comment.
Wow, yes, totally slipped my mind. useMemo is ridiculous here for simple consts, thanks for the catch
| @@ -15,3 +15,5 @@ | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
|
|
|||
| export { default as ExploreTable } from './ExploreTable'; | |||
| export type { ManageColumnsComboBoxOption } from './ManageColumnsComboBox/ManageColumnsComboBox'; | |||
There was a problem hiding this comment.
This is correct, but be aware that if the exports were to include non-types, you'd want to only preface the types themselves, otherwise you would not be able to use the non-type members for anything other than static type analysis.
// myFunc will not be emitted in js, can only be used for type info
export type { MyType, myFunc } from 'utils';
// Can use myFunc for either, as it will be emitted in js
export { type MyType, myFunc } from 'utils';| @@ -267,12 +267,18 @@ export type GraphNode = { | |||
| label: string; | |||
| kind: string; | |||
| objectId: string; | |||
| objectid?: string; | |||
There was a problem hiding this comment.
Is it possible to manually transform these keys? I feel like this is really compromising the type's integrity, specifically this key which is just a lower-case version of another key.
There was a problem hiding this comment.
Chatted about this on slack; this key comes from the properties object. We now type the properties object separately, and export a SpreadNodeWithProperties interface to find this compromise.
| descendent_count?: number | null; | ||
| properties?: Record<string, any>; | ||
| }; | ||
| } & Record<string, any>; |
There was a problem hiding this comment.
This addendum effectively says that a GraphNode object can have any combination of key-value pairs outside the definition here, really weakening the type. Could you elaborate on the issue you encountered that lead you to this? If this is to solve a specific case, we may be better served converting this to a interface or creating an extended type as to not so drastically hinder the original type.
| @@ -285,15 +291,15 @@ export type GraphEdge = { | |||
| impactPercent?: number; | |||
| exploreGraphId?: string; | |||
| data?: Record<string, any>; | |||
| }; | |||
| } & Record<string, any>; | |||
There was a problem hiding this comment.
Same thing here. We should avoid this, if possible. Maybe there is another solution.
Description
useSigmaExploreGraphreturn value to include node_keys and nodes.Motivation and Context
Users should be able to
Resolves: BED-6021
(Also resolves BED-6020 and BED-6018)
How Has This Been Tested?
Tests have been updates and changes have been tested manually.
Screenshots (if appropriate):
Screen.Recording.2025-07-02.at.2.52.44.PM.mov
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Documentation