feat: Add Explore snap-to-grid BED-9229 - #3137
Conversation
justin-prime1
added
enhancement
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds deterministic grid-snapping utilities, collision-free Sigma drag behavior, layout resnapping, lifecycle cleanup, and an Explore view toggle connected to ChangesSnap-to-grid graph interaction
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ExploreUser
participant GraphView
participant SigmaChart
participant GraphEvents
participant snapToGrid
ExploreUser->>GraphView: Toggle snap-to-grid
GraphView->>SigmaChart: Pass snapToGridEnabled
SigmaChart->>GraphEvents: Register graph events
GraphEvents->>snapToGrid: Snap graph positions
snapToGrid-->>GraphEvents: Return collision-free positions
GraphEvents-->>SigmaChart: Update and refresh graph
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
cmd/ui/src/components/SigmaChart/GraphEvents.tsx (2)
93-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid a shared mutable
Setin the default drag metadata.
DEFAULT_DRAGGED_METAis a module-level constant. Every reset assigns the sameSetinstance to state. The current code always replacesoccupiedGridPointsinstead of mutating it, so behavior is correct today. A later change that mutates the set would leak entries into every subsequent drag. Build the default with a factory to remove the hazard.♻️ Proposed refactor
-const DEFAULT_DRAGGED_META = { +const createDefaultDraggedMeta = (): DragMetadata => ({ id: null, cancelNextClick: false, offset: null, origin: null, occupiedGridPoints: new Set<string>(), -}; +});Replace each
DEFAULT_DRAGGED_METAusage withcreateDefaultDraggedMeta().🤖 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 93 - 99, Replace the shared DEFAULT_DRAGGED_META object with a createDefaultDraggedMeta() factory that returns a fresh metadata object and Set on every call, then update all DEFAULT_DRAGGED_META usages to invoke the factory when initializing or resetting drag state.
300-308: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe occupied cell set can be stale at release.
downNodesnapshotsoccupiedGridPointsat drag start. The snap on release uses that snapshot. If node positions change during the drag, for example through a layout run or a graph update, the snapshot no longer matches the graph. The released node can then settle onto an occupied cell.Recomputing the occupied cells inside
mouseupremoves the stale window and keeps the per-drag cost the same.🤖 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 300 - 308, Update the mouseup release logic to recompute occupied grid points from the current graph state immediately before snapping, instead of using the occupiedGridPoints snapshot captured by downNode. Preserve the existing release generation and drag reset behavior while ensuring the released node cannot snap onto a cell occupied after the drag began.cmd/ui/src/views/Explore/snapToGrid.test.ts (1)
61-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting placement quality, not only uniqueness.
The dense test confirms 5,000 unique cells. It does not bound how far nodes travel from the origin cell. A regression that scatters nodes across a very large area still passes. Add an assertion on the maximum offset to lock in compact packing.
🤖 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/views/Explore/snapToGrid.test.ts` around lines 61 - 69, Extend the dense collision test around snapPositionsToGrid to assert a maximum distance or coordinate offset from the original origin cell in addition to uniqueness. Compute the largest resulting x/y offset and require it to stay within a compact packing bound appropriate for 5,000 nodes.cmd/ui/src/views/Explore/GraphView.tsx (1)
314-334: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBind the tooltip to the button so keyboard users receive it.
MUI
Tooltipattaches its focus and hover handlers to its direct child. The direct child here is a<span>, and a plain<span>does not receive keyboard focus. A keyboard user who tabs toGraphButtontherefore never sees the tooltip text. The wrapper is only required when the child is disabled, which is not the case here.
aria-labelandaria-pressedstill convey the control and its state, so this is a polish item rather than a blocker.♻️ Proposed refactor
{!displayTable && ( <Tooltip title={`${isSnapToGridEnabled ? 'Disable' : 'Enable'} snap to grid`} placement='top'> - <span> - <GraphButton - aria-label='Snap to grid' - ... - /> - </span> + <GraphButton + aria-label='Snap to grid' + ... + /> </Tooltip> )}
GraphButtonmust forward its ref for this to work. Verify that before applying.🤖 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/views/Explore/GraphView.tsx` around lines 314 - 334, Update the Tooltip usage in the displayTable control block to make GraphButton its direct child instead of wrapping it in a span, so Tooltip focus handlers reach the keyboard-focusable button. Verify that GraphButton forwards its ref; preserve the existing tooltip text, accessibility attributes, state styling, and click behavior.
🤖 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/GraphEvents.test.tsx`:
- Around line 196-224: Move fake-timer cleanup out of the test body and add an
afterEach hook alongside the existing beforeEach in GraphEvents.test.tsx that
always calls vi.useRealTimers(); remove the trailing vi.useRealTimers() call
from the rapid re-grab test.
In `@cmd/ui/src/views/Explore/snapToGrid.test.ts`:
- Line 46: Update both Set assertions in snapToGrid tests to access the Set’s
size property and use toBe(3), including the assertions near lines 46 and 68; do
not use toHaveLength for Set values.
In `@cmd/ui/src/views/Explore/snapToGrid.ts`:
- Around line 51-67: Update findAvailableGridPosition to validate position.x and
position.y before entering its infinite search loop. Normalize any non-finite
coordinate to a finite fallback value, then use the normalized coordinates for
roundToGrid so offsets can produce distinct candidate cells and the existing
occupied-grid lookup remains unchanged.
---
Nitpick comments:
In `@cmd/ui/src/components/SigmaChart/GraphEvents.tsx`:
- Around line 93-99: Replace the shared DEFAULT_DRAGGED_META object with a
createDefaultDraggedMeta() factory that returns a fresh metadata object and Set
on every call, then update all DEFAULT_DRAGGED_META usages to invoke the factory
when initializing or resetting drag state.
- Around line 300-308: Update the mouseup release logic to recompute occupied
grid points from the current graph state immediately before snapping, instead of
using the occupiedGridPoints snapshot captured by downNode. Preserve the
existing release generation and drag reset behavior while ensuring the released
node cannot snap onto a cell occupied after the drag began.
In `@cmd/ui/src/views/Explore/GraphView.tsx`:
- Around line 314-334: Update the Tooltip usage in the displayTable control
block to make GraphButton its direct child instead of wrapping it in a span, so
Tooltip focus handlers reach the keyboard-focusable button. Verify that
GraphButton forwards its ref; preserve the existing tooltip text, accessibility
attributes, state styling, and click behavior.
In `@cmd/ui/src/views/Explore/snapToGrid.test.ts`:
- Around line 61-69: Extend the dense collision test around snapPositionsToGrid
to assert a maximum distance or coordinate offset from the original origin cell
in addition to uniqueness. Compute the largest resulting x/y offset and require
it to stay within a compact packing bound appropriate for 5,000 nodes.
🪄 Autofix
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: fc6433c1-f1a0-40d8-a5c5-53624515a452
📒 Files selected for processing (7)
cmd/ui/src/components/SigmaChart/GraphEvents.test.tsxcmd/ui/src/components/SigmaChart/GraphEvents.tsxcmd/ui/src/components/SigmaChart/SigmaChart.tsxcmd/ui/src/views/Explore/GraphView.test.tsxcmd/ui/src/views/Explore/GraphView.tsxcmd/ui/src/views/Explore/snapToGrid.test.tscmd/ui/src/views/Explore/snapToGrid.ts
| const snapped = snapPositionsToGrid(positions); | ||
|
|
||
| expect(snapped).toEqual(snapPositionsToGrid(reverseOrder)); | ||
| expect(new Set(Object.values(snapped).map(({ x, y }) => `${x}:${y}`))).toHaveLength(3); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Vitest toHaveLength work with Set size?
💡 Result:
No, Vitest's toHaveLength matcher does not work with Set objects [1][2][3]. The toHaveLength matcher specifically checks for the existence and value of a .length property on the received object [1][3]. Because Set objects in JavaScript use a .size property to report the number of elements instead of a .length property, toHaveLength will not function as expected when passed a Set [1][3]. To assert the size of a Set in Vitest, you should instead use toBe or toEqual to check the .size property directly: typescript import { expect, test } from 'vitest' test('check set size', () => { const mySet = new Set([1, 2, 3]); expect(mySet.size).toBe(3); })
Citations:
- 1: https://vitest.dev/api/expect.html
- 2: https://v0.vitest.dev/api/expect
- 3: https://github.com/vitest-dev/vitest/blob/206e8cff/docs/api/expect.md
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
fd -i 'snapToGrid.test.ts|package.json|pnpm-lock.yaml|yarn.lock|package-lock.json' .
printf '%s\n' '--- relevant test lines ---'
test_file="$(fd -i -t f 'snapToGrid.test.ts' . | head -n 1)"
if [ -n "$test_file" ]; then
cat -n "$test_file" | sed -n '35,75p'
fi
printf '%s\n' '--- Vitest declarations ---'
rg -n --glob 'package.json' --glob '*lock*' --glob '*.yaml' --glob '*.yml' '"vitest"|vitest@' . | head -n 80Repository: SpecterOps/BloodHound
Length of output: 2814
🌐 Web query:
Vitest 4.1.7 toHaveLength matcher Set size implementation
💡 Result:
In Vitest, the toHaveLength matcher is specifically designed to check for a.length property on an object [1][2]. It is not designed to work with the Set data structure, which uses a.size property rather than.length [3][4]. Because Set objects do not have a.length property, using toHaveLength(n) on a Set will fail [1][2]. The matcher implementation in Vitest calls this.have.length(length), which performs a direct check for the existence and value of the.length property [3][4]. To check the number of elements in a Set in Vitest, you should check the.size property directly using the toBe matcher: expect(mySet.size).toBe(n);
Citations:
- 1: https://vitest.dev/api/expect.html
- 2: https://github.com/vitest-dev/vitest/blob/v4.1.10/docs/api/expect.md
- 3: https://github.com/vitest-dev/vitest/blob/da0ade249722df7556638f50a9db01cf01e6a2f8/packages/expect/src/jest-expect.ts
- 4: https://github.com/vitest-dev/vitest/blob/206e8cff/packages/expect/src/jest-expect.ts
Assert the Set size directly.
toHaveLength checks .length, while Set uses .size. Update both assertions at lines 46 and 68 to check new Set(...).size with toBe.
🤖 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/views/Explore/snapToGrid.test.ts` at line 46, Update both Set
assertions in snapToGrid tests to access the Set’s size property and use
toBe(3), including the assertions near lines 46 and 68; do not use toHaveLength
for Set values.
1 participant
Related BHE implementation: https://github.com/SpecterOps/bloodhound-enterprise/pull/1726
Description
Adds an optional, session-local Snap to grid control to the Explore graph. When enabled, the current graph aligns to a deterministic grid, nodes follow the pointer freely while dragged, and released nodes settle visibly into the nearest collision-free grid cell.
The implementation uses BHCE's native Sigma and Graphology capabilities. It preserves free-form behavior while disabled, re-applies grid alignment after supported layout operations and graph replacement, and cancels stale settlement transitions during rapid re-grab, disable, layout changes, graph replacement, and unmount.
Blast radius / risk: Limited to client-side Explore graph controls, Sigma drag handling, layout coordination, and node positioning. The state is not persisted. There are no API, database, migration, authorization, or collection changes. Primary risks are interaction regressions during drag/settlement and additional positioning work when enabled; disabled-mode tests verify that graph-wide occupancy work is avoided.
Test changes: Adds focused coverage for grid rounding, deterministic collision resolution, dense collision allocation, pointer-following drag behavior, animated settlement, rapid re-grab, enable/disable behavior, graph replacement, unmount cleanup, layout integration, control state and styling, keyboard operation, and table-view visibility. No expectations were weakened or removed to accommodate product regressions; one redundant toggle test that did not assert its claimed layout behavior was removed before submission.
Rollback: Revert this PR. The feature stores no persistent state and requires no migration, configuration rollback, or deployment ordering.
Review size: 770 reviewable changed lines — 291 product and 479 tests. Independent review accepted this as one cohesive interaction change with its renderer and lifecycle coverage.
Motivation and Context
Resolves BED-9229
Explore users need a way to organize manually manipulated graphs without losing the existing free-form workflow. This adds a desktop-icon-style grid mode while keeping free-form positioning as the default.
How Has This Been Tested?
yarn workspace bloodhound-ui test GraphEvents.test.tsx GraphView.test.tsx snapToGrid.test.ts --run— 29/29 passed.just prepare-for-codereviewpassed on clean head5f73cb16ef39eff8c20c25f9234a588dc1b4e27d.Not directly validated: touch-specific dragging, release outside the Sigma canvas, large real-world graph profiling, and narrow-viewport layout.
Screenshots (optional):
Not included because no stable PR-accessible capture URL is available. Browser interaction evidence is summarized above.
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes