Conversation
maffkipp
added
the
user interface
WalkthroughGenerates a new frontend-facing AD pathfinding edges function, adds runtime fetching/merging of built-in + OpenGraph edge categories (useEdgeCategories), replaces static pathfinding filter defaults with dynamic initialization and empty-filter handling, and extends the client with getEdgeTypes + EdgeType response types. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as EdgeFilteringDialog (UI)
participant Hook as useEdgeCategories
participant FeatureAPI as Feature Flag API
participant SchemaAPI as Graph Schema API
participant Query as react-query
UI->>Hook: call useEdgeCategories()
Hook->>Query: request feature flag (opengraph_pathfinding)
Query->>FeatureAPI: evaluate flag
FeatureAPI-->>Query: flag value
Query-->>Hook: flag result
alt feature enabled
Hook->>Query: fetch /api/v2/graph-schema/edges
Query->>SchemaAPI: GET /api/v2/graph-schema/edges
SchemaAPI-->>Query: EdgeType[] response
Query-->>Hook: edge types
Hook->>Hook: filterUnneededTypes(...) & mapEdgeTypesToCategory(...)
Hook->>Hook: merge OpenGraph category with BUILTIN_EDGE_CATEGORIES
else feature disabled
Hook->>Hook: use BUILTIN_EDGE_CATEGORIES only
end
Hook-->>UI: return { isLoading, isError, edgeCategories }
UI->>UI: render categories or loading state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
| GenerateTypeScriptStringEnum(root, "ActiveDirectoryKindProperties", schema.Properties) | ||
|
|
||
| GenerateTypeScriptPathfindingEdgesFn(root, "ActiveDirectoryPathfindingEdges", "ActiveDirectoryRelationshipKind", schema.PathfindingRelationships) | ||
| GenerateTypeScriptPathfindingEdgesFn(root, "ActiveDirectoryPathfindingEdgesMatchFrontend", "ActiveDirectoryRelationshipKind", schema.PathfindingRelationshipsMatchFrontend) |
There was a problem hiding this comment.
Adding this list to TS schemagen output so that we can validate our hardcoded edge categories in the UI contain the correct edges.
| } from './utils'; | ||
|
|
||
| // Only need to create our default filters once | ||
| const DEFAULT_FILTERS = createPathFilterString(INITIAL_FILTER_TYPES); |
There was a problem hiding this comment.
Default filters are no longer needed due to the addition of the only_traversable query param that we now use to automatically filter out non-traversable edges.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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/hooks/useExploreGraph/usePathfindingFilters.ts (1)
28-44: Gateinitialize()to prevent incomplete edge filters during async category loading.The filter button calls
initialize()without checkingisLoading. If clicked whileedgeCategoriesare still loading,getInitialPathFilters(edgeCategories)operates on incomplete data (missing OpenGraph edges), resulting in incomplete filter options. Either disable the filter button untilisLoading === false, or add a loading check insideinitialize()to defer sync until categories load.
🤖 Fix all issues with AI agents
In
`@packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/EdgeFilter/useEdgeCategories.test.ts`:
- Around line 62-67: The test is asserting objects but getEdgeListFromCategory
returns string[]; update the assertions to check string values directly: in
useEdgeCategories.test.ts replace the objectContaining check on edgeList with
direct string assertions (e.g., expect(edgeList).not.toContain(<edgeName>) or
expect(edgeList).toEqual(expect.arrayContaining([ 'edgeName1', 'edgeName2' ])))
and keep the length assertion; apply the same fix to the other occurrence around
lines 92-97 so all checks assert against edge names returned by
getEdgeListFromCategory(hook.result.current.edgeCategories).
In
`@packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/EdgeFilter/useEdgeCategories.ts`:
- Around line 25-42: The memoized edgeCategories currently only depends on
edgeTypesQuery.data and can continue to append cached OpenGraph edges when the
feature flag toggles off; update the useMemo for edgeCategories to explicitly
check openGraphFeatureFlag?.enabled (in addition to filtering with
filterUneededTypes) before appending mapEdgeTypesToCategory results to
BUILTIN_EDGE_CATEGORIES, and add openGraphFeatureFlag?.enabled to the dependency
array so the memo recalculates when the flag changes; reference
useFeatureFlag/openGraphFeatureFlag, edgeTypesQuery, useMemo,
BUILTIN_EDGE_CATEGORIES, mapEdgeTypesToCategory, and filterUneededTypes when
making this change.
🧹 Nitpick comments (6)
packages/javascript/bh-shared-ui/src/hooks/useExploreGraph/queries/pathfindingSearch.ts (1)
46-46: Theenabledcondition is redundant.At line 32, you already return
{ enabled: false }when!primarySearch || !searchType || !secondarySearch. If execution reaches line 46, all three are guaranteed to be truthy, making this check always evaluate totrue.Suggested simplification
return { ...sharedGraphQueryOptions, queryKey: [ExploreGraphQueryKey, searchType, primarySearch, secondarySearch, filter], queryFn: ({ signal }) => { return apiClient .getShortestPathV2(primarySearch, secondarySearch, filter, { signal }) .then((res) => res.data); }, - enabled: !!(searchType && primarySearch && secondarySearch), + enabled: true, };packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/EdgeFilter/EdgeFilteringDialog.test.tsx (1)
71-75: Avoid brittle category indexing in tests.Indexing
BUILTIN_EDGE_CATEGORIES[0]assumes ordering; a category insert/reorder would break these assertions. Consider looking up bycategoryNameinstead.♻️ Suggested tweak
- const activeDirectorySubcategories = BUILTIN_EDGE_CATEGORIES[0].subcategories; + const activeDirectorySubcategories = + BUILTIN_EDGE_CATEGORIES.find((c) => c.categoryName === 'Active Directory')?.subcategories ?? []; @@ - const edgeTypes = BUILTIN_EDGE_CATEGORIES[0].subcategories[0].edgeTypes; + const edgeTypes = activeDirectorySubcategories?.[0]?.edgeTypes ?? [];Also applies to: 112-115
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/EdgeFilter/edgeCategories.test.tsx (1)
37-42:Set.prototype.symmetricDifferencerequires Node 20+ and lacks TypeScript typings in versions <5.5.The repository runs Node 22 in CI and targets
ESNext, so the method works at runtime. However, the@ts-ignoresuppresses a legitimate type error in TypeScript 5.1.6. This can be cleaned up by either upgrading TypeScript to 5.5+ or using a manual symmetric difference implementation to avoid the type suppression.Manual implementation option
-function getDifferenceCount(a: string[] | undefined | null, b: string[] | undefined | null) { - const setA = new Set(a); - const setB = new Set(b); - - // `@ts-ignore`: can be removed once typescript is upgraded to >= v5.5 - return setA.symmetricDifference(setB).size; -} +function getDifferenceCount(a: string[] | undefined | null, b: string[] | undefined | null) { + const setA = new Set(a ?? []); + const setB = new Set(b ?? []); + + let diff = 0; + for (const item of setA) if (!setB.has(item)) diff++; + for (const item of setB) if (!setA.has(item)) diff++; + return diff; +}packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/EdgeFilter/utils.ts (3)
22-43: Stabilize subcategory/edge ordering for consistent UI and tests.API ordering isn’t guaranteed; preserving insertion order can lead to UI jitter and snapshot flakiness. If ordering isn’t meaningful, sort subcategories and edge names for determinism.
♻️ Suggested update
- return { - categoryName, - subcategories: Array.from(subcategories.values()), - }; + const sortedSubcategories = Array.from(subcategories.values()) + .map((subcategory) => ({ + ...subcategory, + edgeTypes: [...subcategory.edgeTypes].sort(), + })) + .sort((a, b) => a.name.localeCompare(b.name)); + + return { + categoryName, + subcategories: sortedSubcategories, + };
45-48: Fix exported API typo:filterUneededTypes→filterUnneededTypes.Since this is a new export, it’s worth correcting now to avoid a permanent typo in the public surface. Update call sites accordingly.
✏️ Suggested rename
-export const filterUneededTypes = (data: EdgeType[] | undefined): EdgeType[] | undefined => { +export const filterUnneededTypes = (data: EdgeType[] | undefined): EdgeType[] | undefined => { return data?.filter((edge) => !BUILTIN_TYPES.includes(edge.schema_name) && edge.is_traversable); };
50-54: Return an empty array when category is missing.This avoids
undefinedpropagation and simplifies callers; also makes the return type explicit.✅ Safer return shape
-export const getEdgeListFromCategory = (categoryName: string, categories: Category[] = BUILTIN_EDGE_CATEGORIES) => { - const category = categories.find((category) => category.categoryName === categoryName); - return category?.subcategories.flatMap((subcategory) => subcategory.edgeTypes); -}; +export const getEdgeListFromCategory = ( + categoryName: string, + categories: Category[] = BUILTIN_EDGE_CATEGORIES +): string[] => { + const category = categories.find((category) => category.categoryName === categoryName); + return category ? category.subcategories.flatMap((subcategory) => subcategory.edgeTypes) : []; +};
...javascript/bh-shared-ui/src/views/Explore/ExploreSearch/EdgeFilter/useEdgeCategories.test.ts
Show resolved
Hide resolved
...ages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/EdgeFilter/useEdgeCategories.ts
Outdated
Show resolved
Hide resolved
| const setA = new Set(a); | ||
| const setB = new Set(b); | ||
|
|
||
| const result = new Set([...[...setA].filter((x) => !setB.has(x)), ...[...setB].filter((x) => !setA.has(x))]); |
There was a problem hiding this comment.
I was hoping to use the new Set.symmetricDifference() method here but it is unfortunately not yet available in the JS version we are using in CI: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set/symmetricDifference
|
|
||
| // this hook combines our hardcoded edge categories with an OpenGraph category pulled from the API | ||
| export const useEdgeCategories = () => { | ||
| const { data: openGraphFeatureFlag } = useFeatureFlag('opengraph_search'); |
There was a problem hiding this comment.
note: once the opengraph_pathfinding feature flag is added, this will need to be switched over to use that feature flag.
There was a problem hiding this comment.
Thank you for doing this, hopefully will prevent some future bugs! 🙌
| return { | ||
| data: [ | ||
| { | ||
| key: 'opengraph_search', |
There was a problem hiding this comment.
note: this will also need to be updated to opengraph_pathfinding once the backend is updated
| }; | ||
|
|
||
| // removes all built-in and non-traversable edges from a list of edge types | ||
| export const filterUneededTypes = (data: EdgeType[] | undefined): EdgeType[] | undefined => { |
There was a problem hiding this comment.
nit (non-blocking): typo in this function name, should probably beunneeded
sirisjo
left a comment
sirisjo
left a comment
There was a problem hiding this comment.
Tested locally, this looks good to me!
2 participants
Description
Adds support to our Edge Filtering Dialog under Pathfinding Search on the Explore page for filtering by OpenGraph edge kinds. This functionality is currently gated behind the
pathfinding_searchfeature flag.When a user has uploaded valid OpenGraph edge kinds, they will now show up in the dialog under a new category called "OpenGraph". Underneath that heading, there will be additional categories for the different schemas these edges are a part of. To be valid, an edge must:
is_traversableset totrueadandaz.Motivation and Context
Resolves BED-6725
How Has This Been Tested?
Additional tests added that cover the new behavior added and that the feature flag is gating behavior correctly
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Improvements
API Enhancements
✏️ Tip: You can customize this high-level summary in your review settings.