Replace selection methods with config-driven highlighting and outlining#217
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughReplaces imperative selection with config-driven highlighting/outline; adds stateless GPU-backed rect/polygon "find" queries and relationship queries; converts single-channel status textures to multi-channel point/link status; defers hover callbacks to a mouseout→mouseover ordering and updates callback flags. Changes
Sequence DiagramsequenceDiagram
participant User
participant GraphAPI as Graph
participant Hover as HoverDetection
participant GPU as GPURenderer
participant Store
participant Config as Config/Store
User->>GraphAPI: pointer move / click
GraphAPI->>Hover: findHoveredPoint()/findHoveredLine()
Hover->>GPU: run hover/search shader & read FBO
GPU-->>Hover: pixel hits → indices
Hover-->>GraphAPI: return { mouseout, mouseover }
GraphAPI->>GraphAPI: run mouseout callbacks
GraphAPI->>GraphAPI: run mouseover callbacks (with isHighlighted/isOutlined)
GraphAPI->>Config: setConfigPartial({ highlightedPointIndices, outlinedPointIndices, highlightedLinkIndices })
Config->>Store: update Sets & ring color
Store->>GPU: updatePointStatus() / updateLinkStatus()
GPU->>GPU: regenerate multi-channel status textures
GPU-->>User: render updated frame
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
README.md (1)
34-50:⚠️ Potential issue | 🟡 MinorTypeScript quick-start snippet should avoid nullable/implicit types.
Line 37 (
querySelector) is nullable and needs validation before use. Line 49 has an implicit callback parameter type that should be explicitly annotated.✏️ Suggested doc fix
-const div = document.querySelector('div') +const div = document.querySelector('div') +if (!(div instanceof HTMLDivElement)) { + throw new Error('Expected a target <div> element') +} const config = { @@ - onClick: (pointIndex) => { console.log('Clicked point index: ', pointIndex) }, + onClick: (pointIndex: number | undefined) => { console.log('Clicked point index: ', pointIndex) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 34 - 50, The README snippet uses document.querySelector('div') which returns nullable; update the code that initializes and uses div (the variable from document.querySelector) to validate it before use (e.g., check for null and handle or throw) or obtain a non-null element safely so Graph is only instantiated when div is present; also explicitly annotate the onClick callback parameter in the config (the onClick function in the config object) with its type (e.g., pointIndex: number) instead of relying on an implicit any/implicit type.src/modules/Lines/draw-curve-line.vert (1)
187-199:⚠️ Potential issue | 🟠 MajorInclude focused width in the picking pass.
Line 187 only applies the fixed 5px hover padding during index-buffer rendering. If
focusedLinkWidthIncreaseis set above 5, the focused link renders wider than its hover/click geometry, so interacting with the visible stroke can miss.💡 Possible fix
if (renderMode > 0.0) { // Add 5 pixels padding for better hover detection linkWidthPx += 5.0 / transformationMatrix[0][0]; + if (focusedLinkIndex == linkIndex) { + linkWidthPx += focusedLinkWidthIncrease / transformationMatrix[0][0]; + } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Lines/draw-curve-line.vert` around lines 187 - 199, The picking-pass branch (renderMode > 0.0) only adds a fixed 5px padding to linkWidthPx, so focused links that use focusedLinkWidthIncrease can be visually wider than their pickable area; modify the renderMode > 0.0 block to also add the same focused and hovered width increases (dividing by transformationMatrix[0][0]) when hoveredLinkIndex == linkIndex and when focusedLinkIndex == linkIndex so linkWidthPx matches what's used in the visible pass (use the same symbols: renderMode, linkWidthPx, transformationMatrix, hoveredLinkIndex, focusedLinkIndex, hoveredLinkWidthIncrease, focusedLinkWidthIncrease, linkIndex).
🧹 Nitpick comments (3)
src/stories/shapes/image-example/index.ts (1)
199-201: Deduplicate highlighted points before applying config (optional).If
getNeighboringPointIndices()includes the clicked point, duplicates may be passed downstream unnecessarily.♻️ Suggested refinement
- const highlightedPointIndices = [pointIndex, ...neighboringPoints] + const highlightedPointIndices = [...new Set([pointIndex, ...neighboringPoints])] const highlightedLinkIndices = graph.getConnectedLinkIndices(highlightedPointIndices)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stories/shapes/image-example/index.ts` around lines 199 - 201, The highlightedPointIndices array may contain duplicates if getNeighboringPointIndices() returns the clicked point; before calling graph.getConnectedLinkIndices and graph.setConfigPartial, deduplicate the highlightedPointIndices (e.g., create a Set from [pointIndex, ...neighboringPoints] and convert back to an array) so graph.getConnectedLinkIndices(highlightedPointIndices) and graph.setConfigPartial({ highlightedPointIndices, highlightedLinkIndices }) receive unique point IDs; keep the same variable names highlightedPointIndices and highlightedLinkIndices so downstream calls remain unchanged.src/stories/clusters/polygon-selection/index.ts (1)
28-29: Guard polygon query until graph is ready.To avoid edge-case calls before initialization completes, add a readiness check before
findPointsInPolygon().🛡️ Suggested guard
const polygonSelection = new PolygonSelection(div, (polygonPoints) => { + if (!graph.isReady) return const indices = graph.findPointsInPolygon(polygonPoints) graph.setConfigPartial({ highlightedPointIndices: indices }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stories/clusters/polygon-selection/index.ts` around lines 28 - 29, Guard the polygon query so we don't call graph.findPointsInPolygon before the graph is initialized: before calling graph.findPointsInPolygon(polygonPoints) check a readiness flag or method on the graph (e.g., graph.isReady(), graph.ready, or graph.initialized) and only call findPointsInPolygon and then graph.setConfigPartial({ highlightedPointIndices: indices }) when that readiness check passes; if the graph isn't ready, either no-op or subscribe to the graph's ready event and run the same code then.src/modules/Points/find-hovered-point.vert (1)
71-72: Minor naming inconsistency for clarity.The local variable
greyoutStatussamples from texturepointStatus, which now contains both greyout (R) and outlined (G) channels. Consider renaming tostatusfor consistency withdraw-points.vert:- vec4 greyoutStatus = texture(pointStatus, (pointIndices + 0.5) / pointsTextureSize); - float isHighlighted = (greyoutStatus.r == 0.0) ? 1.0 : 0.0; + vec4 status = texture(pointStatus, (pointIndices + 0.5) / pointsTextureSize); + float isHighlighted = (status.r == 0.0) ? 1.0 : 0.0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Points/find-hovered-point.vert` around lines 71 - 72, Rename the local GLSL variable greyoutStatus to a more general name like status in the vertex shader so it reflects that pointStatus texture contains multiple channels (R=greyout, G=outlined) and to match naming in draw-points.vert; update the subsequent usage of isHighlighted (and any other references) to read from status instead of greyoutStatus while keeping the texture lookup using pointStatus and pointsTextureSize unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/index.ts`:
- Around line 941-949: findPointsInRect relies on GPU resources (searchFbo,
program/uniform setup) produced by Points.initPrograms() and
Points.updatePositions(), so calling graph.ready alone can return stale/empty
results; add a preparation step before using this.points.searchFbo to ensure
those GPU resources and pending updates are created/flushed (mirror the
preparation used in findPointsInPolygon). Concretely, add/Call a method like
this.points.prepareForSearch() (implement it to call/await
Points.initPrograms(), Points.updatePositions(), and flush any pending
render/update commands) at the start of findPointsInRect (and symmetrically in
findPointsInPolygon) so searchFbo and related uniforms are valid before
readPixels/extractIndicesFromPixels are invoked.
---
Outside diff comments:
In `@README.md`:
- Around line 34-50: The README snippet uses document.querySelector('div') which
returns nullable; update the code that initializes and uses div (the variable
from document.querySelector) to validate it before use (e.g., check for null and
handle or throw) or obtain a non-null element safely so Graph is only
instantiated when div is present; also explicitly annotate the onClick callback
parameter in the config (the onClick function in the config object) with its
type (e.g., pointIndex: number) instead of relying on an implicit any/implicit
type.
In `@src/modules/Lines/draw-curve-line.vert`:
- Around line 187-199: The picking-pass branch (renderMode > 0.0) only adds a
fixed 5px padding to linkWidthPx, so focused links that use
focusedLinkWidthIncrease can be visually wider than their pickable area; modify
the renderMode > 0.0 block to also add the same focused and hovered width
increases (dividing by transformationMatrix[0][0]) when hoveredLinkIndex ==
linkIndex and when focusedLinkIndex == linkIndex so linkWidthPx matches what's
used in the visible pass (use the same symbols: renderMode, linkWidthPx,
transformationMatrix, hoveredLinkIndex, focusedLinkIndex,
hoveredLinkWidthIncrease, focusedLinkWidthIncrease, linkIndex).
---
Nitpick comments:
In `@src/modules/Points/find-hovered-point.vert`:
- Around line 71-72: Rename the local GLSL variable greyoutStatus to a more
general name like status in the vertex shader so it reflects that pointStatus
texture contains multiple channels (R=greyout, G=outlined) and to match naming
in draw-points.vert; update the subsequent usage of isHighlighted (and any other
references) to read from status instead of greyoutStatus while keeping the
texture lookup using pointStatus and pointsTextureSize unchanged.
In `@src/stories/clusters/polygon-selection/index.ts`:
- Around line 28-29: Guard the polygon query so we don't call
graph.findPointsInPolygon before the graph is initialized: before calling
graph.findPointsInPolygon(polygonPoints) check a readiness flag or method on the
graph (e.g., graph.isReady(), graph.ready, or graph.initialized) and only call
findPointsInPolygon and then graph.setConfigPartial({ highlightedPointIndices:
indices }) when that readiness check passes; if the graph isn't ready, either
no-op or subscribe to the graph's ready event and run the same code then.
In `@src/stories/shapes/image-example/index.ts`:
- Around line 199-201: The highlightedPointIndices array may contain duplicates
if getNeighboringPointIndices() returns the clicked point; before calling
graph.getConnectedLinkIndices and graph.setConfigPartial, deduplicate the
highlightedPointIndices (e.g., create a Set from [pointIndex,
...neighboringPoints] and convert back to an array) so
graph.getConnectedLinkIndices(highlightedPointIndices) and
graph.setConfigPartial({ highlightedPointIndices, highlightedLinkIndices })
receive unique point IDs; keep the same variable names highlightedPointIndices
and highlightedLinkIndices so downstream calls remain unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13995ac2-494d-473e-89a8-3731ccae4f60
📒 Files selected for processing (22)
README.mdmigration-notes.mdsrc/config.tssrc/helper.tssrc/index.tssrc/modules/GraphData/index.tssrc/modules/Lines/draw-curve-line.vertsrc/modules/Lines/index.tssrc/modules/Points/draw-highlighted.vertsrc/modules/Points/draw-points.fragsrc/modules/Points/draw-points.vertsrc/modules/Points/find-hovered-point.vertsrc/modules/Points/find-points-in-polygon.fragsrc/modules/Points/find-points-in-rect.fragsrc/modules/Points/index.tssrc/modules/Store/index.tssrc/stories/2. configuration.mdxsrc/stories/3. api-reference.mdxsrc/stories/beginners/basic-set-up/index.tssrc/stories/clusters/polygon-selection/index.tssrc/stories/shapes/image-example/index.tssrc/variables.ts
Stukova
force-pushed
the
feat/config-driven-highlighting
branch
from
c61f036 to
a08cbe6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/modules/Store/index.ts (1)
41-41: TypesearchAreaas a fixed 2-point tuple.This is currently inferred as
number[][], so malformed shapes can still compile and only fail deeper in the rect-search path. Pinning it to[[number, number], [number, number]]keeps the new search contract checked at compile time.♻️ Suggested change
- public searchArea = [[0, 0], [0, 0]] + public searchArea: [[number, number], [number, number]] = [[0, 0], [0, 0]]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Store/index.ts` at line 41, The public field searchArea is currently inferred as number[][] allowing malformed shapes; change its declaration to have the fixed tuple type [[number, number], [number, number]] and keep the same initial value so the compiler enforces two 2-number points. Update the Store class's public searchArea declaration to use the explicit tuple type [[number, number], [number, number]] wherever it's declared and adjust any assignments/usages to satisfy that shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/Lines/index.ts`:
- Around line 685-699: When link highlighting is cleared or there are no links,
release the old GPU texture to avoid pinned memory: in the branches handling
!linksNumber and highlightedLinkIndices === undefined (where linkStatusTexture
and ensureLinkStatusPlaceholder are referenced), call the texture
destructor/release on this.linkStatusTexture, set this.linkStatusTexture to
null/undefined, and set this.linkStatusTextureSize = 0; keep the existing
ensureLinkStatusPlaceholder behavior for creating a 1×1 placeholder when needed.
In `@src/modules/Points/index.ts`:
- Around line 1664-1668: findPointsInPolygon currently proceeds for 1- or
2-point inputs; add an early check at the top of the findPointsInPolygon method
to reject degenerate polygons by returning false when the polygon vertex count
is < 3 (i.e., inspect the polygon vertex array used by this method — e.g. the
polygon vertex buffer/array or the property that backs polygonPathTexture — and
if its length/vertexCount < 3 return false before any GPU resources or commands
are used). Ensure the check runs before referencing
this.findPointsInPolygonCommand, this.searchFbo, this.currentPositionTexture, or
this.polygonPathTexture so the GPU pass is never launched for invalid polygons.
---
Nitpick comments:
In `@src/modules/Store/index.ts`:
- Line 41: The public field searchArea is currently inferred as number[][]
allowing malformed shapes; change its declaration to have the fixed tuple type
[[number, number], [number, number]] and keep the same initial value so the
compiler enforces two 2-number points. Update the Store class's public
searchArea declaration to use the explicit tuple type [[number, number],
[number, number]] wherever it's declared and adjust any assignments/usages to
satisfy that shape.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 195e7150-d22c-43e3-b892-8603e2824121
📒 Files selected for processing (22)
README.mdmigration-notes.mdsrc/config.tssrc/helper.tssrc/index.tssrc/modules/GraphData/index.tssrc/modules/Lines/draw-curve-line.vertsrc/modules/Lines/index.tssrc/modules/Points/draw-highlighted.vertsrc/modules/Points/draw-points.fragsrc/modules/Points/draw-points.vertsrc/modules/Points/find-hovered-point.vertsrc/modules/Points/find-points-in-polygon.fragsrc/modules/Points/find-points-in-rect.fragsrc/modules/Points/index.tssrc/modules/Store/index.tssrc/stories/2. configuration.mdxsrc/stories/3. api-reference.mdxsrc/stories/beginners/basic-set-up/index.tssrc/stories/clusters/polygon-selection/index.tssrc/stories/shapes/image-example/index.tssrc/variables.ts
✅ Files skipped from review due to trivial changes (4)
- src/variables.ts
- src/modules/Points/find-points-in-polygon.frag
- README.md
- src/index.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- src/stories/clusters/polygon-selection/index.ts
- src/helper.ts
- src/modules/Points/draw-highlighted.vert
- src/stories/shapes/image-example/index.ts
- src/modules/Points/find-points-in-rect.frag
- src/modules/Points/draw-points.frag
- src/modules/GraphData/index.ts
- src/stories/beginners/basic-set-up/index.ts
- migration-notes.md
- src/stories/2. configuration.mdx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/GraphData/index.ts`:
- Around line 387-399: In getConnectedPointIndices, validate that each supplied
linkIndex is an integer before using it to index this.links: for the
single-number and array cases reject or skip any non-integer (use
Number.isInteger(linkIndex) or equivalent) in addition to the existing range
check (linksNumber) so fractional values like 1.5 are ignored; keep the existing
checks for this.links undefined and the source/target extraction
(this.links[linkIndex * 2], this.links[linkIndex * 2 + 1]) intact and only
execute them for indices that pass both the integer and range guards.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa23d2e7-82a4-487d-9705-d940c2587e4b
📒 Files selected for processing (1)
src/modules/GraphData/index.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/index.ts (1)
968-970: Avoid warning on every incomplete polygon preview.
findPointsInPolygon()is a likely building block for interactive lasso/polygon selection, so callers may invoke it repeatedly while the path still has 1–2 vertices. Returning[]is already enough here; the warning will just spam the console.💡 Possible fix
- if (polygonPath.length < 3) { - console.warn('Polygon path requires at least 3 points to form a polygon.') - return [] - } + if (polygonPath.length < 3) return []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 968 - 970, The console.warn in findPointsInPolygon that triggers when polygonPath.length < 3 should be removed to avoid spamming during interactive lasso/polygon construction; simply return [] as is. Locate the check in findPointsInPolygon referencing polygonPath.length and delete the warning call (console.warn('Polygon path requires at least 3 points to form a polygon.')), leaving the early return intact so callers get an empty array without noisy logs.src/stories/beginners/explore-connections/index.ts (1)
165-173:graph.zoom(0.9)is likely temporary here.
render()still runs the init-fit path wheninitialZoomLevelis unset, so this zoom call is likely to be overwritten on first paint. If the goal is a looser initial fit, preferfitViewPadding; if the goal is a fixed starting scale, useinitialZoomLevelor disablefitViewOnInit.Based on learnings, Cosmos Graph initializes by fitting the view, which triggers an onZoom event on load.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stories/beginners/explore-connections/index.ts` around lines 165 - 173, The explicit graph.zoom(0.9) call is being overwritten by Graph.render()'s init-fit behavior; instead, update the Graph construction/config (the config passed to new Graph(...)) to either set initialZoomLevel to fix the starting scale, set fitViewPadding to achieve a looser initial fit, or set fitViewOnInit:false to prevent the automatic fit — remove the temporary graph.zoom(0.9) line and adjust the config (initialZoomLevel, fitViewPadding, or fitViewOnInit) so the desired initial zoom is applied before/at render().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/index.ts`:
- Around line 968-970: The console.warn in findPointsInPolygon that triggers
when polygonPath.length < 3 should be removed to avoid spamming during
interactive lasso/polygon construction; simply return [] as is. Locate the check
in findPointsInPolygon referencing polygonPath.length and delete the warning
call (console.warn('Polygon path requires at least 3 points to form a
polygon.')), leaving the early return intact so callers get an empty array
without noisy logs.
In `@src/stories/beginners/explore-connections/index.ts`:
- Around line 165-173: The explicit graph.zoom(0.9) call is being overwritten by
Graph.render()'s init-fit behavior; instead, update the Graph
construction/config (the config passed to new Graph(...)) to either set
initialZoomLevel to fix the starting scale, set fitViewPadding to achieve a
looser initial fit, or set fitViewOnInit:false to prevent the automatic fit —
remove the temporary graph.zoom(0.9) line and adjust the config
(initialZoomLevel, fitViewPadding, or fitViewOnInit) so the desired initial zoom
is applied before/at render().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 384d26e8-1fbf-4a4e-bd40-127a7a68e123
📒 Files selected for processing (5)
src/index.tssrc/stories/beginners.stories.tssrc/stories/beginners/explore-connections/data-gen.tssrc/stories/beginners/explore-connections/index.tssrc/stories/beginners/explore-connections/style.css
✅ Files skipped from review due to trivial changes (1)
- src/stories/beginners/explore-connections/style.css
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/stories/beginners.stories.ts (1)
53-62: Minor inconsistency in sourceCode naming conventions.The
Actionsstory uses'data-gen'(line 59) without the.tsextension, whileExploreConnectionsuses'data-gen.ts'(line 169) with the extension. Other existing stories in this file also use.tsextensions (e.g.,'data.ts','config.ts').Consider aligning to the existing convention with
.tsextensions for consistency in the displayed source code tabs.✨ Suggested fix for consistency
export const Actions: Story = { ...createStory(actions), parameters: { sourceCode: [ { name: 'Story', code: actionsStoryRaw }, { name: 'style.css', code: actionsStoryCssRaw }, - { name: 'data-gen', code: actionsStoryDataGenRaw }, + { name: 'data-gen.ts', code: actionsStoryDataGenRaw }, ], }, }Also applies to: 163-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stories/beginners.stories.ts` around lines 53 - 62, The sourceCode tab names are inconsistent: update the parameters.sourceCode entries for the Actions story (export const Actions) to use the '.ts' extension (change the 'data-gen' name to 'data-gen.ts') so it matches the convention used elsewhere (e.g., ExploreConnections and other stories); also make the same change for the other story block mentioned (the data-gen entry around ExploreConnections) by renaming its sourceCode name from 'data-gen' to 'data-gen.ts' to keep tab naming consistent across stories.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/stories/beginners.stories.ts`:
- Around line 53-62: The sourceCode tab names are inconsistent: update the
parameters.sourceCode entries for the Actions story (export const Actions) to
use the '.ts' extension (change the 'data-gen' name to 'data-gen.ts') so it
matches the convention used elsewhere (e.g., ExploreConnections and other
stories); also make the same change for the other story block mentioned (the
data-gen entry around ExploreConnections) by renaming its sourceCode name from
'data-gen' to 'data-gen.ts' to keep tab naming consistent across stories.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02db85fa-50c1-4ec2-8942-b145145efe13
📒 Files selected for processing (6)
README.mdsrc/stories/1. welcome.mdxsrc/stories/beginners.stories.tssrc/stories/beginners/actions/data-gen.tssrc/stories/beginners/actions/index.tssrc/stories/beginners/actions/style.css
✅ Files skipped from review due to trivial changes (1)
- src/stories/1. welcome.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
rokotyan
approved these changes
Apr 13, 2026
…g, outlining, and focusing Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
… methods Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
…g, and focusing Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Stukova
force-pushed
the
feat/config-driven-highlighting
branch
from
bcb32f1 to
b51f47c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.ts (1)
1948-1990:⚠️ Potential issue | 🟠 MajorFire
mouseoutwhen the hovered item changes within the same type.Moving directly from point A→point B or link A→link B currently only emits
mouseoverfor B. Consumers relying ononPointMouseOut/onLinkMouseOutfor cleanup will miss the old item’s exit event.Proposed fix
if (pointSize > 0) { if (this.store.hoveredPoint === undefined || this.store.hoveredPoint.index !== hoveredIndex) { + if (this.store.hoveredPoint !== undefined) isMouseout = true isMouseover = true } this.store.hoveredPoint = { index: hoveredIndex, position: [pointX, pointY], @@ if (hoveredLineIndex >= 0) { - if (this.store.hoveredLinkIndex !== hoveredLineIndex) isMouseover = true + if (this.store.hoveredLinkIndex !== hoveredLineIndex) { + if (this.store.hoveredLinkIndex !== undefined) isMouseout = true + isMouseover = true + } this.store.hoveredLinkIndex = hoveredLineIndex } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 1948 - 1990, The hover handlers currently only set mouseover when switching items of the same type, missing a mouseout for the previous item; update both the point-hover logic (the block that reads hoveredIndex and this.store.hoveredPoint) and findHoveredLine (which sets hoveredLineIndex and this.store.hoveredLinkIndex) so that if an existing hovered item exists and its index differs from the newly-detected index you set mouseout = true before replacing the stored hover and still set mouseover = true for the new one; specifically, in the point code check if this.store.hoveredPoint !== undefined && this.store.hoveredPoint.index !== hoveredIndex to set isMouseout, and in findHoveredLine check if this.store.hoveredLinkIndex !== undefined && this.store.hoveredLinkIndex !== hoveredLineIndex to set isMouseout, then update this.store.hoveredPoint / this.store.hoveredLinkIndex afterwards.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/Points/draw-points.frag`:
- Around line 269-273: The smoothstep calls can get equal-edge arguments when r
== 0.0 (or outlineWidth == 0), which is undefined; fix by using stable, non-zero
edges before calling smoothstep: compute a safe radius (e.g., rSafe = max(r,
1e-6)) and a safe outline width (e.g., wSafe = max(outlineWidth, 1e-6)), then
use those (rSafe and wSafe) when computing outerEdge and innerEdge (keep
ringSmoothing as-is) so smoothstep never receives identical edge values; update
the references to pointCoord/r and outlineWidth in the outerEdge/innerEdge
calculations accordingly so ringAlpha remains outerEdge * innerEdge.
In `@src/modules/Points/draw-points.vert`:
- Around line 141-146: The outlined point scaling can exceed the capped size
from calculatePointSize(): after you multiply overallSizeValue by
outlineRingScale inside the isOutlined branch, clamp overallSizeValue back to
the same cap used earlier (e.g., min(overallSizeValue, maxPointSize * ratio) or
the uniform representing the hardware max if available) before assigning to
gl_PointSize; update the block that checks isOutlined and uses outlineRingScale
to apply this clamp so the outline never exceeds the supported point size.
In `@src/stories/beginners/explore-connections/data-gen.ts`:
- Around line 48-59: The childrenPerLevel index is off-by-one for depths
starting at 2: in buildBranch use childrenPerLevel[depth - 2] (falling back to
default) instead of childrenPerLevel[depth - 1] so depth 2 maps to
childrenPerLevel[0], depth 3 to childrenPerLevel[1], etc.; update the lookup
where numChildren is computed (and the identical logic around lines 121-124) in
buildBranch to use depth - 2 and keep the existing nullish default (e.g., ?? 2).
---
Outside diff comments:
In `@src/index.ts`:
- Around line 1948-1990: The hover handlers currently only set mouseover when
switching items of the same type, missing a mouseout for the previous item;
update both the point-hover logic (the block that reads hoveredIndex and
this.store.hoveredPoint) and findHoveredLine (which sets hoveredLineIndex and
this.store.hoveredLinkIndex) so that if an existing hovered item exists and its
index differs from the newly-detected index you set mouseout = true before
replacing the stored hover and still set mouseover = true for the new one;
specifically, in the point code check if this.store.hoveredPoint !== undefined
&& this.store.hoveredPoint.index !== hoveredIndex to set isMouseout, and in
findHoveredLine check if this.store.hoveredLinkIndex !== undefined &&
this.store.hoveredLinkIndex !== hoveredLineIndex to set isMouseout, then update
this.store.hoveredPoint / this.store.hoveredLinkIndex afterwards.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 330889ef-4cb2-4de2-b5d5-5fe17c87f8e3
📒 Files selected for processing (29)
README.mdmigration-notes.mdsrc/config.tssrc/helper.tssrc/index.tssrc/modules/GraphData/index.tssrc/modules/Lines/draw-curve-line.vertsrc/modules/Lines/index.tssrc/modules/Points/draw-highlighted.vertsrc/modules/Points/draw-points.fragsrc/modules/Points/draw-points.vertsrc/modules/Points/find-hovered-point.vertsrc/modules/Points/find-points-in-polygon.fragsrc/modules/Points/find-points-in-rect.fragsrc/modules/Points/index.tssrc/modules/Store/index.tssrc/stories/1. welcome.mdxsrc/stories/2. configuration.mdxsrc/stories/3. api-reference.mdxsrc/stories/beginners.stories.tssrc/stories/beginners/actions/data-gen.tssrc/stories/beginners/actions/index.tssrc/stories/beginners/actions/style.csssrc/stories/beginners/explore-connections/data-gen.tssrc/stories/beginners/explore-connections/index.tssrc/stories/beginners/explore-connections/style.csssrc/stories/clusters/polygon-selection/index.tssrc/stories/shapes/image-example/index.tssrc/variables.ts
✅ Files skipped from review due to trivial changes (6)
- src/stories/1. welcome.mdx
- src/stories/beginners/explore-connections/style.css
- src/stories/clusters/polygon-selection/index.ts
- src/modules/Points/find-points-in-rect.frag
- README.md
- src/modules/Lines/index.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/variables.ts
- src/stories/shapes/image-example/index.ts
- src/modules/Points/find-points-in-polygon.frag
- src/helper.ts
- src/stories/beginners.stories.ts
- migration-notes.md
- src/stories/beginners/actions/index.ts
Large arrays like highlightedPointIndices were being shallow-copied on every setConfig/setConfigPartial call, which is expensive for frequent updates. User-provided arrays are now stored by reference — callers are responsible for not mutating them after passing. Default arrays (pulled from the module-level defaultConfigValues singleton) are still cloned to prevent shared references between Graph instances. Updated JSDoc and inline comments to explain the reasoning. Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/config.ts (1)
717-729:createDefaultConfigshallow-clone is adequate for current defaults — worth a brief note for future maintainers.
Array.isArray(value) ? [...value] : valueis a shallow copy, which is sufficient today because every array default indefaultConfigValues(e.g.linkVisibilityDistanceRange: [50, 150],simulationLinkDistRandomVariationRange: [1, 1.2]) contains only primitives. If a future default were to contain objects or nested arrays (e.g. the[[left,bottom],[right,top]]shape offitViewByPointsInRect), inner references would be shared across Graph instances again. Consider a short code comment calling out that constraint, or switching tostructuredClonefor arrays to be future-proof.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.ts` around lines 717 - 729, The function createDefaultConfig currently shallow-clones arrays (Array.isArray(value) ? [...value] : value) which is fine for current primitive-only defaults but will leak nested object/array references if future defaults include non-primitives; add a short comment above createDefaultConfig calling out that this is a shallow, one-level clone and that nested objects/arrays will be shared, and either replace the shallow clone with a deep clone using structuredClone(value) for array values (falling back to a JSON deep-clone if structuredClone isn't available) or explicitly document the constraint next to defaultConfigValues so future maintainers know to update createDefaultConfig when adding nested defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/config.ts`:
- Around line 717-729: The function createDefaultConfig currently shallow-clones
arrays (Array.isArray(value) ? [...value] : value) which is fine for current
primitive-only defaults but will leak nested object/array references if future
defaults include non-primitives; add a short comment above createDefaultConfig
calling out that this is a shallow, one-level clone and that nested
objects/arrays will be shared, and either replace the shallow clone with a deep
clone using structuredClone(value) for array values (falling back to a JSON
deep-clone if structuredClone isn't available) or explicitly document the
constraint next to defaultConfigValues so future maintainers know to update
createDefaultConfig when adding nested defaults.
… childrenPerLevel index - draw-points.frag: use rSafe/wSafe (max with 1e-6) before smoothstep calls in the outlined ring so edge0 < edge1 is always guaranteed, even when r = 0 (center fragment) or outlineWidth = 0 (current hardcoded value) - draw-points.vert: clamp overallSizeValue back to maxPointSize * ratio after the outlineRingScale multiply so gl_PointSize never silently exceeds the hardware limit; point body is unaffected, only the ring narrows at the cap - explore-connections/data-gen.ts: fix off-by-one in buildBranch — childrenPerLevel[depth - 2] correctly maps depth 2→[0], 3→[1], 4→[2]; depth 3 nodes were getting 2 children instead of the intended 3 Signed-off-by: Stukova Olya <stukova.o@gmail.com>
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.
Replace the imperative selection API with config-driven properties for controlling point and link visual states. Visual state now lives in config and is controlled via
setConfig()/setConfigPartial().Point and link highlighting are now independent — greying out points does not automatically grey out links.
Semantics:
[]vsundefinedFor highlighting,
undefinedmeans "no highlighting active" and[]means "highlighting active with everything greyed out." ForoutlinedPointIndicesthe distinction doesn't apply — outlining is additive (no greyout), so[]andundefinedare visually equivalent; only the listed indices ever get a ring.Before / after
Removed methods
selectPointByIndex()/selectPointsByIndices()setConfigPartial({ highlightedPointIndices })selectPointsInRect()findPointsInRect()+setConfigPartial({ highlightedPointIndices })selectPointsInPolygon()findPointsInPolygon()+setConfigPartial({ highlightedPointIndices })unselectPoints()setConfigPartial({ highlightedPointIndices: undefined, highlightedLinkIndices: undefined })getSelectedIndices()Renamed methods (signatures changed)
getAdjacentIndices(index)getNeighboringPointIndices(pointIndices)number | number[], returns deduplicatednumber[]getPointsInRect(selection)findPointsInRect(rect)number[]instead ofFloat32Array; must be called afterawait graph.readygetPointsInPolygon(polygonPath)findPointsInPolygon(polygonPath)number[]instead ofFloat32Array; must be called afterawait graph.readyNew config properties
highlightedPointIndicesnumber[] | undefinedundefinedoutlinedPointIndicesnumber[] | undefinedundefinedoutlinedPointRingColorstring | RGBA'white'highlightedLinkIndicesnumber[] | undefinedundefinedfocusedLinkIndexnumber | undefinedundefinedfocusedLinkWidthIncreasenumber5New methods
getConnectedLinkIndices(pointIndices)number[]getConnectedPointIndices(linkIndices)number[]Both accept
number \| number[]. Out-of-range indices are skipped silently rather than throwing.Callback changes
onPointMouseOverisSelected→isHighlighted; new 5th paramisOutlinedHover event ordering fix
Hover detection now uses a two-phase approach: state is updated first for both points and links, then callbacks fire in the correct order —
mouseoutalways fires beforemouseoverwhen transitioning between element types (e.g. link → point). Previously, moving from a link to a point would fireonPointMouseOverbeforeonLinkMouseOut.Summary by CodeRabbit
New Features
Breaking Changes
Documentation