feat: Full Path Highlight - BED-8246#2812
Open
specter-flq wants to merge 2 commits into
Open
Conversation
specter-flq
added
the
user interface
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds graph highlighting utilities, unit tests, Redux state/actions/types for an explore-graph highlight flag, and integrates memoized full-path highlight computations into SigmaChart's GraphEvents to drive selective node/edge dimming with theme-aware shader parameters. ChangesExplore graph highlight dimming
Sequence DiagramsequenceDiagram
participant Client as GraphEvents(Component)
participant Redux as Redux Selector
participant IsInGraph as getIsHighlightedItemInGraph
participant FullPath as getFullPathHighlightedEntities
participant Graph as GraphologyGraph
Client->>Redux: read highlightedItem, isExploreGraphHighlight, theme/darkMode
Client->>IsInGraph: (graph, highlightedItem)
IsInGraph->>Graph: hasNode / hasEdge
Graph-->>IsInGraph: boolean
Client->>FullPath: (graph, highlightedItem) [if in graph]
FullPath->>Graph: outEdges / inEdges / extremities
Graph-->>FullPath: nodes/edges
FullPath-->>Client: highlightedNodeIds, highlightedEdgeIds
Client->>Client: update node/edge reducers -> apply dimming/shaders
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
specter-flq
force-pushed
the
BED-8246--full-path-highlight
branch
from
c18fbb6 to
89a5da2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/ui/src/components/SigmaChart/GraphEvents.tsx (2)
291-303:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply computed
isDimmedto reducer output.
isDimmedis calculated but never returned, so non-highlighted nodes/edges are not actually dimmed.💡 Proposed fix
nodeReducer: (node, data) => { @@ return { ...data, + isDimmed, highlighted: node === highlightedItem, inverseSqrtZoomRatio: 1 / Math.sqrt(camera.ratio), }; }, @@ const newData: Attributes = { ...data, hidden: false, + isDimmed, highlighted: edge === highlightedItem, inverseSqrtZoomRatio: 1 / Math.sqrt(camera.ratio), };Also applies to: 305-318
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ui/src/components/SigmaChart/GraphEvents.tsx` around lines 291 - 303, The computed isDimmed flag in the nodeReducer (and likewise in edgeReducer) is not included in the returned data object, so dimming never takes effect; update the return value of nodeReducer (and the analogous return in edgeReducer) to spread the existing data and include a property (e.g., dimmed: isDimmed) or set highlighted/dimmed appropriately so the UI can read the dim state (reference nodeReducer, edgeReducer, highlightedItem, highlightedNodeIds, and isExploreGraphHighlight).
19-19:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix missing
useMemoimport (build-breaking)
useMemois used incmd/ui/src/components/SigmaChart/GraphEvents.tsx(e.g., lines ~281 and ~286) but isn’t imported from React; theuseEffectblock around ~289 is already syntactically correct and already has a dependency array.Proposed fix
-import { forwardRef, useCallback, useEffect, useImperativeHandle, useLayoutEffect, useState } from 'react'; +import { forwardRef, useCallback, useEffect, useImperativeHandle, useLayoutEffect, useMemo, useState } from 'react';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ui/src/components/SigmaChart/GraphEvents.tsx` at line 19, The file uses React's useMemo (around the hooks at ~281 and ~286) but the top import list in GraphEvents.tsx only imports forwardRef, useCallback, useEffect, useImperativeHandle, useLayoutEffect, useState; add useMemo to that import list so useMemo is imported from React (i.e., update the import statement to include useMemo) and re-run the build to confirm the missing-import error is resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cmd/ui/src/components/SigmaChart/GraphEvents.tsx`:
- Around line 291-303: The computed isDimmed flag in the nodeReducer (and
likewise in edgeReducer) is not included in the returned data object, so dimming
never takes effect; update the return value of nodeReducer (and the analogous
return in edgeReducer) to spread the existing data and include a property (e.g.,
dimmed: isDimmed) or set highlighted/dimmed appropriately so the UI can read the
dim state (reference nodeReducer, edgeReducer, highlightedItem,
highlightedNodeIds, and isExploreGraphHighlight).
- Line 19: The file uses React's useMemo (around the hooks at ~281 and ~286) but
the top import list in GraphEvents.tsx only imports forwardRef, useCallback,
useEffect, useImperativeHandle, useLayoutEffect, useState; add useMemo to that
import list so useMemo is imported from React (i.e., update the import statement
to include useMemo) and re-run the build to confirm the missing-import error is
resolved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f113ee71-4d2d-48ba-be0d-03d7a11c416a
📒 Files selected for processing (7)
cmd/ui/src/components/SigmaChart/GraphEvents.tsxcmd/ui/src/components/SigmaChart/utils.test.tsxcmd/ui/src/components/SigmaChart/utils.tsxcmd/ui/src/ducks/global/actions.tscmd/ui/src/ducks/global/reducer.tscmd/ui/src/ducks/global/types.tscmd/ui/src/store.ts
Contributor
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 (2)
cmd/ui/src/components/SigmaChart/GraphEvents.tsx (2)
295-305:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWire
isDimmedinto reducer output.Line 295 and Line 309 compute
isDimmed, but the returned node/edge data never uses it, so the fade behavior is effectively a no-op.🔧 Proposed fix
return { ...data, highlighted: node === highlightedItem, + dimmed: Boolean(isDimmed), inverseSqrtZoomRatio: 1 / Math.sqrt(camera.ratio), }; }, @@ const newData: Attributes = { ...data, hidden: false, highlighted: edge === highlightedItem, + dimmed: Boolean(isDimmed), inverseSqrtZoomRatio: 1 / Math.sqrt(camera.ratio), };Also applies to: 309-320
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ui/src/components/SigmaChart/GraphEvents.tsx` around lines 295 - 305, The computed isDimmed flag (from isExploreGraphHighlight, highlightedItem, highlightedNodeIds, isHighlightedItemInGraph) is never added to the reducer output so fade behavior is a no-op; update the reducer return for node and edge data (the object that currently spreads data and sets highlighted and inverseSqrtZoomRatio) to include isDimmed: isDimmed so downstream renderers can use it, and do the same for the other block around lines 309–320 where an equivalent isDimmed is computed.
19-20:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winImport missing hooks before use in GraphEvents.tsx
useMemois used (around lines 281–289) but isn’t imported in thereactimport (line 19) → TS compile break; adduseMemoto the React import.useTheme()is used (around line 96) but there’s nouseThemeimport from@mui/material/styles; ensureuseThemeis imported from the correct module so it’s in scope.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ui/src/components/SigmaChart/GraphEvents.tsx` around lines 19 - 20, The file GraphEvents.tsx is missing two imports used later: add useMemo to the React import list (alongside forwardRef, useCallback, etc.) so the hook referenced around the useMemo block compiles, and import useTheme from '`@mui/material/styles`' so the call to useTheme() around the theme-related logic is in scope; update the import statements at the top of GraphEvents.tsx to include useMemo in the React import and add a named import { useTheme } from '`@mui/material/styles`'.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/ui/src/components/SigmaChart/utils.test.tsx`:
- Around line 1-2: The test file cmd/ui/src/components/SigmaChart/utils.test.tsx
is missing the required license/header which is failing CI; add the project's
standard license header comment block at the very top of this file (above all
imports) so the header is present for all test files — no change to the existing
imports or tests (e.g., MultiDirectedGraph import or references to
getFullPathHighlightedEntities and getIsHighlightedItemInGraph) is required.
In `@cmd/ui/src/components/SigmaChart/utils.tsx`:
- Around line 1-2: Add the project's standard license header to the top of the
file to match the automated header generation; open
cmd/ui/src/components/SigmaChart/utils.tsx and insert the required license block
as the very first lines so CI's Code Generation Check no longer flags the file,
ensuring the header format exactly matches other files in the repo (same comment
style and year/owner placeholders).
---
Outside diff comments:
In `@cmd/ui/src/components/SigmaChart/GraphEvents.tsx`:
- Around line 295-305: The computed isDimmed flag (from isExploreGraphHighlight,
highlightedItem, highlightedNodeIds, isHighlightedItemInGraph) is never added to
the reducer output so fade behavior is a no-op; update the reducer return for
node and edge data (the object that currently spreads data and sets highlighted
and inverseSqrtZoomRatio) to include isDimmed: isDimmed so downstream renderers
can use it, and do the same for the other block around lines 309–320 where an
equivalent isDimmed is computed.
- Around line 19-20: The file GraphEvents.tsx is missing two imports used later:
add useMemo to the React import list (alongside forwardRef, useCallback, etc.)
so the hook referenced around the useMemo block compiles, and import useTheme
from '`@mui/material/styles`' so the call to useTheme() around the theme-related
logic is in scope; update the import statements at the top of GraphEvents.tsx to
include useMemo in the React import and add a named import { useTheme } from
'`@mui/material/styles`'.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 774d9ef1-e162-4fc2-9927-497c0a86458d
📒 Files selected for processing (7)
cmd/ui/src/components/SigmaChart/GraphEvents.tsxcmd/ui/src/components/SigmaChart/utils.test.tsxcmd/ui/src/components/SigmaChart/utils.tsxcmd/ui/src/ducks/global/actions.tscmd/ui/src/ducks/global/reducer.tscmd/ui/src/ducks/global/types.tscmd/ui/src/store.ts
specter-flq
force-pushed
the
BED-8246--full-path-highlight
branch
from
89a5da2 to
d1e77ee
Compare
specter-flq
force-pushed
the
BED-8246--full-path-highlight
branch
from
5da52de to
bbb65b2
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/ui/src/components/SigmaChart/utils.tsx (1)
45-47: ⚡ Quick winReplace
shift()-based BFS queues to avoid quadratic traversal cost.Using
Array.shift()in both BFS loops can degrade to O(n²) on large paths. Switch to index-based queue iteration.♻️ Proposed fix
- const outboundQueue: string[] = [highlightedItem]; - while (outboundQueue.length > 0) { - const current = outboundQueue.shift()!; + const outboundQueue: string[] = [highlightedItem]; + for (let i = 0; i < outboundQueue.length; i++) { + const current = outboundQueue[i]; graph.outEdges(current).forEach((edge) => { highlightedEdgeIds.add(edge); const neighbor = graph.target(edge); if (!highlightedNodeIds.has(neighbor)) { highlightedNodeIds.add(neighbor); outboundQueue.push(neighbor); } }); } // Inbound BFS: follow edges pointing toward (target → source). // Uses its own visited set so outbound discoveries don't cause early stops. const inboundVisited = new Set<string>([highlightedItem]); - const inboundQueue: string[] = [highlightedItem]; - while (inboundQueue.length > 0) { - const current = inboundQueue.shift()!; + const inboundQueue: string[] = [highlightedItem]; + for (let i = 0; i < inboundQueue.length; i++) { + const current = inboundQueue[i]; graph.inEdges(current).forEach((edge) => { highlightedEdgeIds.add(edge); const neighbor = graph.source(edge); if (!inboundVisited.has(neighbor)) { inboundVisited.add(neighbor); highlightedNodeIds.add(neighbor); inboundQueue.push(neighbor); } }); }Also applies to: 61-63
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ui/src/components/SigmaChart/utils.tsx` around lines 45 - 47, The BFS loops use Array.shift() which can lead to O(n²) behavior; replace both shift()-based queues (the outboundQueue used around highlightedItem and the other inbound/outbound queue at the 61-63 region) with index-based iteration: initialize an index (e.g., let i = 0) and loop while i < queue.length, retrieving the current element via queue[i++] instead of queue.shift(); update any null/non-null assertions (e.g., the current variable) and preserve the existing queue push logic and types so the traversal semantics remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/ui/src/components/SigmaChart/utils.tsx`:
- Around line 45-47: The BFS loops use Array.shift() which can lead to O(n²)
behavior; replace both shift()-based queues (the outboundQueue used around
highlightedItem and the other inbound/outbound queue at the 61-63 region) with
index-based iteration: initialize an index (e.g., let i = 0) and loop while i <
queue.length, retrieving the current element via queue[i++] instead of
queue.shift(); update any null/non-null assertions (e.g., the current variable)
and preserve the existing queue push logic and types so the traversal semantics
remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 67731e87-51a0-41a8-9421-725b567b7735
📒 Files selected for processing (6)
cmd/ui/src/components/SigmaChart/GraphEvents.tsxcmd/ui/src/components/SigmaChart/utils.test.tsxcmd/ui/src/components/SigmaChart/utils.tsxcmd/ui/src/ducks/global/actions.tscmd/ui/src/ducks/global/reducer.tscmd/ui/src/ducks/global/types.ts
specter-flq
force-pushed
the
BED-8246--full-path-highlight
branch
from
14a6b65 to
1ad31a2
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
Added full path highlight to graph. When user clicks on entity it fades everything but inbound and outbound relationships
Added the ability to disable this feature via local storage. It reads of a local storage key and will default to true. Will only disable the feature if it's explicitly set to false.
Motivation and Context
Resolves BED-8246
How Has This Been Tested?
Existing and new tests passed.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Tests