WIP Config-driven highlighting, outlining, and focusing#211
Closed
Stukova wants to merge 14 commits into
Closed
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 APIs with config-driven highlighting/outlining for points and links; adds synchronous spatial queries and connectivity queries; updates hover callback to report Changes
Sequence DiagramsequenceDiagram
participant User as User/App
participant Graph as Graph Core
participant Points as Points Module
participant Lines as Lines Module
participant Shaders as GPU Shaders
User->>Graph: setConfigPartial({ highlightedPointIndices, outlinedPointIndices, highlightedLinkIndices, focusedLinkIndex })
activate Graph
Graph->>Points: updatePointStatus()
activate Points
Points->>Points: build/resize pointStatusTexture (R=highlighted, G=outlined)
Points-->>Graph: pointStatusTexture ready
deactivate Points
Graph->>Lines: updateLinkStatus()
activate Lines
Lines->>Lines: build/resize linkStatusTexture (R=link status)
Lines-->>Graph: linkStatusTexture ready
deactivate Lines
Graph->>Shaders: bind pointStatusTexture, linkStatusTexture, set highlight/focus uniforms
Shaders->>Shaders: sample pointStatus -> derive isHighlighted/isOutlined
Shaders->>Shaders: sample linkStatus -> apply greyout or focused width
Shaders-->>Graph: draw frame
Graph-->>User: rendered frame
deactivate Graph
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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🧪 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 |
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
![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 (5)
src/modules/Lines/index.ts (1)
679-728: Consider potential performance concern withupdateLinkStatusfor large graphs.The method iterates over all links twice when highlighting is active (lines 695-703): once to mark all as greyed out, then again to un-grey highlighted links. For graphs with many links and many highlighted indices, this could be optimized by using a Set for O(1) lookups instead of iterating the
highlightedLinkIndicesarray.♻️ Suggested optimization using Set for faster lookups
public updateLinkStatus (): void { const { device, config, data } = this const linksNumber = data.linksNumber ?? 0 if (!linksNumber) return const { highlightedLinkIndices } = config const hasHighlighting = highlightedLinkIndices !== undefined // Calculate texture size (square texture large enough for all links) const textureSize = Math.ceil(Math.sqrt(linksNumber)) this.linkStatusTextureSize = textureSize const state = new Float32Array(textureSize * textureSize * 4) if (hasHighlighting) { - // Mark all links as greyed out (R=1) - for (let i = 0; i < linksNumber; i++) { - state[i * 4] = 1 - } - // Un-grey highlighted links - for (const idx of highlightedLinkIndices) { - if (idx >= 0 && idx < linksNumber) { - state[idx * 4] = 0 - } - } + // Use Set for O(1) lookup when determining greyout state + const highlightedSet = new Set(highlightedLinkIndices) + for (let i = 0; i < linksNumber; i++) { + // R=1 means greyed out (not highlighted), R=0 means highlighted + state[i * 4] = highlightedSet.has(i) ? 0 : 1 + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Lines/index.ts` around lines 679 - 728, updateLinkStatus currently iterates links twice when highlighting is active (first loop setting state[i*4]=1 for all links, then looping highlightedLinkIndices to reset some), which is O(n*m); convert highlightedLinkIndices to a Set (e.g., const highlightedSet = new Set(highlightedLinkIndices)) and then perform a single pass over linksNumber where you set state[i*4] = highlightedSet.has(i) ? 0 : 1, keeping existing texture-size/creation logic (linkStatusTextureSize, state, copyData, linkStatusTexture.copyImageData, and device.createTexture) unchanged.src/config.ts (1)
166-193: Consider consistency betweenfocusedLinkWidthIncreaseoptionality and its sibling properties.
focusedLinkWidthIncreaseis marked as optional (?) on line 193, whilehoveredLinkWidthIncreaseon line 212 is required. Both have default values. Consider makingfocusedLinkWidthIncreaserequired for consistency, or document why it differs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.ts` around lines 166 - 193, The focusedLinkWidthIncrease property in the config interface is marked optional (focusedLinkWidthIncrease?) while its sibling hoveredLinkWidthIncrease is required despite both having defaults; make them consistent by removing the optional marker from focusedLinkWidthIncrease (i.e., change focusedLinkWidthIncrease?: number to focusedLinkWidthIncrease: number) and ensure any initialization/defaulting code or documentation still provides the default value (5) and that any callers/constructors that construct the config supply or default this field accordingly; alternatively, if you intend it to remain optional, update hoveredLinkWidthIncrease to be optional and document the rationale—adjust the Config type and related initialization logic where focusedLinkWidthIncrease and hoveredLinkWidthIncrease are referenced.src/index.ts (1)
1898-1906: Performance consideration forisHighlightedandisOutlinedlookups.The hover callback uses
Array.includes()which is O(n) for each lookup. For graphs with many highlighted/outlined points, this could cause performance degradation during frequent hover events.Consider caching these as Sets when the config changes, similar to the suggestion for
updateLinkStatus. However, given that hover detection already has throttling (MAX_HOVER_DETECTION_DELAY) and this runs at most once per several frames, the impact may be acceptable for typical use cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 1898 - 1906, The hover handler currently computes isHighlighted/isOutlined using Array.includes on this.config.highlightedPointIndices and outlinedPointIndices each hover (O(n)); cache those arrays as Sets and use Set.has for O(1) lookups: add maintained properties (e.g. highlightedPointIndexSet and outlinedPointIndexSet) that are updated whenever the config changes (or when highlightedPointIndices/outlinedPointIndices are set/updated) and then change the hover code that computes isHighlighted/isOutlined to consult these Sets before calling this.config.onPointMouseOver; ensure you reference and update these new Sets wherever the config or those index arrays are modified (e.g. in the config setter or updateLinkStatus-like helper) and keep store.hoveredPoint.index and onPointMouseOver usage unchanged.src/modules/Points/index.ts (2)
1045-1060: Usedata.pointsNumberas index upper bound for point status updates.Line 1045 uses texture capacity as the max index. That accepts indices beyond actual point count and writes status for padded texels. It’s safer and clearer to clamp against real point count.
♻️ Proposed tweak
-const maxIndex = pointsTextureSize * pointsTextureSize +const maxIndex = this.data.pointsNumber ?? (pointsTextureSize * pointsTextureSize)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Points/index.ts` around lines 1045 - 1060, The code currently derives maxIndex from pointsTextureSize^2 and writes status into padded texels; change logic to use the actual point count in data.pointsNumber as the upper bound. Replace the maxIndex value computed from pointsTextureSize with const maxIndex = data.pointsNumber, update the initial R-channel fill to iterate only over the first maxIndex texels (e.g. loop by texel index setting state[idx*4]=1) instead of iterating the entire state buffer, and keep the highlightedPointIndices/outlinedPointIndices bounds checks using this new maxIndex so you only modify status for real points (affecting hasHighlighting, hasOutlining, state, highlightedPointIndices, outlinedPointIndices).
1434-1494: Consider extracting shared two-pass highlight logic into a helper.The
hasHighlightinglayered pass logic is duplicated in draw and hover search paths (sameskipHighlighted/skipGreyedtoggling). A small helper would reduce drift risk when behavior changes again.Also applies to: 1770-1802
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Points/index.ts` around lines 1434 - 1494, The duplicated two-pass highlighting logic (toggling skipHighlighted/skipGreyed, calling drawUniformStore.setUniforms, drawCommand.setBindings, and drawCommand.draw) should be extracted into a single helper function (e.g., renderWithHighlighting or performTwoPassDraw) and reused from both the draw path and the hover-search path; implement the helper to accept baseVertexUniforms/baseFragmentUniforms, the textures (currentPositionTexture, pointStatusTexture, imageAtlasTexture, imageAtlasCoordsTexture) and renderPass, perform the greyed-first then highlighted-second sequences (setting skipHighlighted/skipGreyed accordingly) and a single-pass fallback when hasHighlighting is false, then replace the duplicated blocks in the methods that call drawUniformStore.setUniforms/drawCommand.setBindings/drawCommand.draw with calls to this new helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@migration-notes.md`:
- Around line 49-60: The migration examples incorrectly recommend using
setConfig() to update highlights, which will reset omitted properties; change
those examples (the ones mentioning selectPointByIndex/selectPointsByIndices,
selectPointsInRect/selectPointsInPolygon, and unselectPoints flows) to call
setConfigPartial({ highlightedPointIndices }) or setConfigPartial({
highlightedPointIndices: undefined, highlightedLinkIndices: undefined }) instead
of setConfig(), and update the example that shows using findPointsInRect() to
call setConfigPartial with the returned highlightedPointIndices to avoid
unintentionally resetting other config fields.
In `@src/modules/Points/index.ts`:
- Around line 1622-1652: The search pass methods can early-return without
updating the framebuffer, causing callers to read stale results—change the
signature of findPointsInRect (and the analogous findPointsInPolygon) to return
a boolean success flag, return false on all early-exits where you currently
"return" (e.g., when !findPointsInRectCommand, !searchFbo, destroyed textures,
etc.), and return true after the renderPass.end() when the draw completes;
ensure callers (the wrappers that invoke findPointsInRect/findPointsInPolygon)
check the boolean and skip any framebuffer readback if false. Use the existing
symbols to locate code: findPointsInRect, findPointsInPolygon,
findPointsInRectCommand, findPointsInRectUniformStore, searchFbo,
currentPositionTexture, sizeTexture.
In `@src/stories/2`. configuration.mdx:
- Around line 20-21: Update the two table rows for the properties
pointGreyoutOpacity and pointGreyoutColor to use the hyphenated compound
adjective "greyed-out" instead of "greyed out" in their descriptions (and apply
the same change to the other occurrence mentioned around the pointGreyout*
entries), ensuring the descriptions read e.g. "Opacity multiplier for greyed-out
points..." and "The color to use for points when they are greyed-out (when
highlighting is active)..." so the phrasing is consistent.
---
Nitpick comments:
In `@src/config.ts`:
- Around line 166-193: The focusedLinkWidthIncrease property in the config
interface is marked optional (focusedLinkWidthIncrease?) while its sibling
hoveredLinkWidthIncrease is required despite both having defaults; make them
consistent by removing the optional marker from focusedLinkWidthIncrease (i.e.,
change focusedLinkWidthIncrease?: number to focusedLinkWidthIncrease: number)
and ensure any initialization/defaulting code or documentation still provides
the default value (5) and that any callers/constructors that construct the
config supply or default this field accordingly; alternatively, if you intend it
to remain optional, update hoveredLinkWidthIncrease to be optional and document
the rationale—adjust the Config type and related initialization logic where
focusedLinkWidthIncrease and hoveredLinkWidthIncrease are referenced.
In `@src/index.ts`:
- Around line 1898-1906: The hover handler currently computes
isHighlighted/isOutlined using Array.includes on
this.config.highlightedPointIndices and outlinedPointIndices each hover (O(n));
cache those arrays as Sets and use Set.has for O(1) lookups: add maintained
properties (e.g. highlightedPointIndexSet and outlinedPointIndexSet) that are
updated whenever the config changes (or when
highlightedPointIndices/outlinedPointIndices are set/updated) and then change
the hover code that computes isHighlighted/isOutlined to consult these Sets
before calling this.config.onPointMouseOver; ensure you reference and update
these new Sets wherever the config or those index arrays are modified (e.g. in
the config setter or updateLinkStatus-like helper) and keep
store.hoveredPoint.index and onPointMouseOver usage unchanged.
In `@src/modules/Lines/index.ts`:
- Around line 679-728: updateLinkStatus currently iterates links twice when
highlighting is active (first loop setting state[i*4]=1 for all links, then
looping highlightedLinkIndices to reset some), which is O(n*m); convert
highlightedLinkIndices to a Set (e.g., const highlightedSet = new
Set(highlightedLinkIndices)) and then perform a single pass over linksNumber
where you set state[i*4] = highlightedSet.has(i) ? 0 : 1, keeping existing
texture-size/creation logic (linkStatusTextureSize, state, copyData,
linkStatusTexture.copyImageData, and device.createTexture) unchanged.
In `@src/modules/Points/index.ts`:
- Around line 1045-1060: The code currently derives maxIndex from
pointsTextureSize^2 and writes status into padded texels; change logic to use
the actual point count in data.pointsNumber as the upper bound. Replace the
maxIndex value computed from pointsTextureSize with const maxIndex =
data.pointsNumber, update the initial R-channel fill to iterate only over the
first maxIndex texels (e.g. loop by texel index setting state[idx*4]=1) instead
of iterating the entire state buffer, and keep the
highlightedPointIndices/outlinedPointIndices bounds checks using this new
maxIndex so you only modify status for real points (affecting hasHighlighting,
hasOutlining, state, highlightedPointIndices, outlinedPointIndices).
- Around line 1434-1494: The duplicated two-pass highlighting logic (toggling
skipHighlighted/skipGreyed, calling drawUniformStore.setUniforms,
drawCommand.setBindings, and drawCommand.draw) should be extracted into a single
helper function (e.g., renderWithHighlighting or performTwoPassDraw) and reused
from both the draw path and the hover-search path; implement the helper to
accept baseVertexUniforms/baseFragmentUniforms, the textures
(currentPositionTexture, pointStatusTexture, imageAtlasTexture,
imageAtlasCoordsTexture) and renderPass, perform the greyed-first then
highlighted-second sequences (setting skipHighlighted/skipGreyed accordingly)
and a single-pass fallback when hasHighlighting is false, then replace the
duplicated blocks in the methods that call
drawUniformStore.setUniforms/drawCommand.setBindings/drawCommand.draw with calls
to this new helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: faf7f56b-1dba-439b-b3ea-85fa95c6f81b
📒 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
4b7f0cd to
712c50a
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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)
src/modules/Lines/draw-curve-line.vert (1)
187-199:⚠️ Potential issue | 🟡 MinorFocused width should affect picking too.
The focus enlargement only applies in normal rendering. The default
focusedLinkWidthIncreasehappens to match the built-in5pxpicking padding, but any larger configured value makes the outer part of the visible stroke non-hoverable/non-clickable.Suggested 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 { // Add pixel increase if this is the hovered link if (hoveredLinkIndex == linkIndex) {🤖 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 branch (renderMode > 0.0) adds a fixed 5px padding to linkWidthPx but omits the configured focus/hover increases, so focused links larger than 5px become unpickable; update the renderMode > 0.0 branch to also add hoveredLinkWidthIncrease/transformationMatrix[0][0] when hoveredLinkIndex == linkIndex and focusedLinkWidthIncrease/transformationMatrix[0][0] when focusedLinkIndex == linkIndex (same checks used in the else branch), ensuring linkWidthPx accounts for both the base picking padding and any configured hover/focus increases for correct hit testing.
🧹 Nitpick comments (3)
src/modules/Points/draw-points.frag (1)
276-284: Consider extracting shared greyout color blending logic.The greyout blending logic here duplicates the pattern from
applyGreyoutToImage(lines 69-77). For consistency and maintainability, consider extracting a helper function:vec3 applyGreyoutToColor(vec3 color) { float blendFactor = 0.65; if (isDarkenGreyout > 0.0) { return mix(color, vec3(0.2), blendFactor); } else { return mix(color, max(backgroundColor.rgb, vec3(0.8)), blendFactor); } }This would allow both
applyGreyoutToImageand the ring rendering to use the same function, reducing the risk of divergence if the greyout behavior changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Points/draw-points.frag` around lines 276 - 284, Extract the duplicated greyout blending into a helper function (e.g., vec3 applyGreyoutToColor(vec3 color)) that uses isDarkenGreyout, backgroundColor and the same blendFactor = 0.65 logic, then replace the ringColor block in draw-points.frag and the existing applyGreyoutToImage implementation to call applyGreyoutToColor(color) so both image and ring rendering share the same behavior and avoid divergence.src/modules/GraphData/index.ts (1)
365-377: Avoid double-traversing fully-contained links.Each qualifying link is visited once from the source side and again from the target side, then deduped by
Set. Iterating a deduped input set over one adjacency direction preserves the current semantics and cuts the work roughly in half for larger selections.♻️ Proposed refactor
public getAdjacentLinkIndices (pointIndices: number | number[]): number[] { - const indices = Array.isArray(pointIndices) ? pointIndices : [pointIndices] - const indexSet = new Set(indices) + const indexSet = new Set(Array.isArray(pointIndices) ? pointIndices : [pointIndices]) const result = new Set<number>() - for (const index of indices) { + for (const index of indexSet) { for (const [targetIndex, linkIndex] of this.sourceIndexToTargetIndices?.[index] ?? []) { if (indexSet.has(targetIndex)) result.add(linkIndex) } - for (const [sourceIndex, linkIndex] of this.targetIndexToSourceIndices?.[index] ?? []) { - if (indexSet.has(sourceIndex)) result.add(linkIndex) - } } return [...result] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/GraphData/index.ts` around lines 365 - 377, The function getAdjacentLinkIndices currently iterates the original indices array and visits the same link twice (once from sourceIndexToTargetIndices and once from targetIndexToSourceIndices) then relies on a Set to dedupe; fix by deduping the input into indexSet/array and when scanning adjacency lists only add a link when you are processing the canonical endpoint (e.g., only add links where sourceIndex <= targetIndex or otherwise compare sourceIndex and targetIndex to choose one direction), so iterate the deduped indices and for each index scan this.sourceIndexToTargetIndices and this.targetIndexToSourceIndices but only add linkIndex if the (sourceIndex,targetIndex) pair satisfies your canonical ordering; this preserves results while halving traversal. Ensure changes are applied inside getAdjacentLinkIndices and reference sourceIndexToTargetIndices / targetIndexToSourceIndices and the result Set.README.md (1)
34-53: Make the Quick Start snippet strict-TypeScript safe if we keep thetsfence.After Line 34 switched this example to TypeScript,
document.querySelector('div')is nullable andpointIndexbecomes implicitanyin strict mode becauseconfigis untyped. Either keep the snippet as JavaScript or add a null check and explicit types so users can paste it into a strict TS project.Suggested docs tweak
import { Graph } from '@cosmos.gl/graph' -const div = document.querySelector('div') +const div = document.querySelector<HTMLDivElement>('div') +if (!div) throw new Error('Graph container not found') + const config = { spaceSize: 4096, simulationFriction: 0.1, // keeps the graph inert @@ - 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 - 53, The TypeScript snippet is not strict-safe: document.querySelector('div') can be null and onClick's pointIndex is implicitly any; update the Quick Start by adding a null-check or non-null assertion for the DOM element (the variable div used when calling new Graph) and add explicit types for config and the onClick handler (e.g., type the config object or at least declare onClick: (pointIndex: number) => void) so the Graph constructor and onClick are valid in strict mode; ensure you reference the existing symbols Graph, div, config, onClick, and pointIndex when making the changes.
🤖 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 679-728: updateLinkStatus returns early when data.linksNumber is
zero, leaving this.linkStatusTexture and this.linkStatusTextureSize stale and
leaking GPU memory; modify updateLinkStatus to detect linksNumber === 0 and,
before returning, if this.linkStatusTexture exists and is not destroyed call
this.linkStatusTexture.destroy(), set this.linkStatusTexture to undefined/null
and set this.linkStatusTextureSize to 0 (or null), ensuring you reference
data.linksNumber, this.linkStatusTexture, this.linkStatusTexture.destroy(), and
this.linkStatusTextureSize when locating where to add the cleanup.
In `@src/modules/Points/draw-points.frag`:
- Around line 269-273: The shader treats r = dot(pointCoord, pointCoord)
(squared radius) but uses outlineWidth as if it were a linear radius, causing a
mismatch; update the fragment logic so comparisons use linear radius: compute
float rLinear = length(pointCoord) and use rLinear in the innerEdge smoothstep
(and adjust outerEdge to use rLinear or keep using r^2 consistently), or
alternatively require outlineWidth to be pre-squared on the CPU and
document/rename it; locate symbols r, pointCoord, outlineWidth, innerEdge,
outerEdge, and ringAlpha and make the chosen change so outlineWidth semantics
match the distance measure used in smoothstep.
---
Outside diff comments:
In `@src/modules/Lines/draw-curve-line.vert`:
- Around line 187-199: The picking branch (renderMode > 0.0) adds a fixed 5px
padding to linkWidthPx but omits the configured focus/hover increases, so
focused links larger than 5px become unpickable; update the renderMode > 0.0
branch to also add hoveredLinkWidthIncrease/transformationMatrix[0][0] when
hoveredLinkIndex == linkIndex and
focusedLinkWidthIncrease/transformationMatrix[0][0] when focusedLinkIndex ==
linkIndex (same checks used in the else branch), ensuring linkWidthPx accounts
for both the base picking padding and any configured hover/focus increases for
correct hit testing.
---
Nitpick comments:
In `@README.md`:
- Around line 34-53: The TypeScript snippet is not strict-safe:
document.querySelector('div') can be null and onClick's pointIndex is implicitly
any; update the Quick Start by adding a null-check or non-null assertion for the
DOM element (the variable div used when calling new Graph) and add explicit
types for config and the onClick handler (e.g., type the config object or at
least declare onClick: (pointIndex: number) => void) so the Graph constructor
and onClick are valid in strict mode; ensure you reference the existing symbols
Graph, div, config, onClick, and pointIndex when making the changes.
In `@src/modules/GraphData/index.ts`:
- Around line 365-377: The function getAdjacentLinkIndices currently iterates
the original indices array and visits the same link twice (once from
sourceIndexToTargetIndices and once from targetIndexToSourceIndices) then relies
on a Set to dedupe; fix by deduping the input into indexSet/array and when
scanning adjacency lists only add a link when you are processing the canonical
endpoint (e.g., only add links where sourceIndex <= targetIndex or otherwise
compare sourceIndex and targetIndex to choose one direction), so iterate the
deduped indices and for each index scan this.sourceIndexToTargetIndices and
this.targetIndexToSourceIndices but only add linkIndex if the
(sourceIndex,targetIndex) pair satisfies your canonical ordering; this preserves
results while halving traversal. Ensure changes are applied inside
getAdjacentLinkIndices and reference sourceIndexToTargetIndices /
targetIndexToSourceIndices and the result Set.
In `@src/modules/Points/draw-points.frag`:
- Around line 276-284: Extract the duplicated greyout blending into a helper
function (e.g., vec3 applyGreyoutToColor(vec3 color)) that uses isDarkenGreyout,
backgroundColor and the same blendFactor = 0.65 logic, then replace the
ringColor block in draw-points.frag and the existing applyGreyoutToImage
implementation to call applyGreyoutToColor(color) so both image and ring
rendering share the same behavior and avoid divergence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f19c9ed9-ccbf-48ed-b292-05efd48e0ba8
📒 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 (2)
- src/modules/Points/find-points-in-polygon.frag
- src/variables.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- src/modules/Points/draw-highlighted.vert
- src/helper.ts
- src/stories/shapes/image-example/index.ts
- src/stories/beginners/basic-set-up/index.ts
- src/stories/clusters/polygon-selection/index.ts
- src/modules/Points/find-points-in-rect.frag
- src/modules/Points/draw-points.vert
- src/modules/Store/index.ts
- migration-notes.md
- src/stories/3. api-reference.mdx
- src/index.ts
- src/modules/Points/index.ts
- src/stories/2. configuration.mdx
Stukova
force-pushed
the
feat/config-driven-highlighting
branch
from
712c50a to
9ea7ffd
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/index.ts (1)
1898-1907: Consider performance ofincludes()for large highlight/outline arrays.For graphs with many highlighted or outlined points, calling
includes()on each hover event performs a linear scan. If arrays are large (thousands of points), this could introduce noticeable latency.Consider converting to
Setat config update time if performance becomes an issue:// In updateStateFromConfig, cache as Set: this._highlightedPointSet = new Set(this.config.highlightedPointIndices)For typical use cases with smaller arrays, the current implementation is fine.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 1898 - 1907, The hover handler currently calls this.config.highlightedPointIndices?.includes(...) and this.config.outlinedPointIndices?.includes(...) on every mouseover, which can be O(n) per event; to fix, cache these arrays as Sets during config updates (e.g. in updateStateFromConfig create this._highlightedPointSet and this._outlinedPointSet from this.config.highlightedPointIndices / outlinedPointIndices) and then change the hover logic to test membership via this._highlightedPointSet.has(idx) and this._outlinedPointSet.has(idx) so hover checks become O(1) while preserving existing behavior when arrays are small.
🤖 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 1898-1907: The hover handler currently calls
this.config.highlightedPointIndices?.includes(...) and
this.config.outlinedPointIndices?.includes(...) on every mouseover, which can be
O(n) per event; to fix, cache these arrays as Sets during config updates (e.g.
in updateStateFromConfig create this._highlightedPointSet and
this._outlinedPointSet from this.config.highlightedPointIndices /
outlinedPointIndices) and then change the hover logic to test membership via
this._highlightedPointSet.has(idx) and this._outlinedPointSet.has(idx) so hover
checks become O(1) while preserving existing behavior when arrays are small.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a5d7702-14aa-45d7-b4bc-3c02e6c588a8
📒 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/modules/Points/find-points-in-polygon.frag
- src/variables.ts
- src/modules/Points/draw-points.frag
- src/modules/Lines/index.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- src/helper.ts
- src/modules/Points/draw-highlighted.vert
- src/stories/clusters/polygon-selection/index.ts
- src/modules/Store/index.ts
- migration-notes.md
- src/modules/Points/find-hovered-point.vert
- src/stories/shapes/image-example/index.ts
- src/modules/Lines/draw-curve-line.vert
- src/stories/2. configuration.mdx
- src/stories/beginners/basic-set-up/index.ts
- src/modules/Points/draw-points.vert
- src/stories/3. api-reference.mdx
- src/modules/Points/find-points-in-rect.frag
Bumps the npm_and_yarn group with 1 update in the / directory: [dompurify](https://github.com/cure53/DOMPurify). Updates `dompurify` from 3.2.6 to 3.3.1 - [Release notes](https://github.com/cure53/DOMPurify/releases) - [Commits](cure53/DOMPurify@3.2.6...3.3.1) --- updated-dependencies: - dependency-name: dompurify dependency-version: 3.3.1 dependency-type: direct:production dependency-group: npm_and_yarn ... Signed-off-by: dependabot[bot] <support@github.com>
…dates Bumps the npm_and_yarn group with 2 updates in the / directory: [dompurify](https://github.com/cure53/DOMPurify) and [flatted](https://github.com/WebReflection/flatted). Updates `dompurify` from 3.3.1 to 3.3.3 - [Release notes](https://github.com/cure53/DOMPurify/releases) - [Commits](cure53/DOMPurify@3.3.1...3.3.3) Updates `flatted` from 3.1.1 to 3.4.2 - [Commits](WebReflection/flatted@v3.1.1...v3.4.2) --- updated-dependencies: - dependency-name: dompurify dependency-version: 3.3.3 dependency-type: direct:production dependency-group: npm_and_yarn - dependency-name: flatted dependency-version: 3.4.2 dependency-type: indirect dependency-group: npm_and_yarn ... Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Nikita Rokotyan <nikita@rokotyan.com>
Stukova
force-pushed
the
feat/config-driven-highlighting
branch
3 times, most recently
from
8fd76de to
277bf12
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 (1)
src/modules/Lines/draw-curve-line.vert (1)
187-199:⚠️ Potential issue | 🟡 MinorApply the focus width to the picking pass too.
focusedLinkWidthIncreaseis only added in the visible pass. InrenderMode > 0.0the hit area still uses the fixed 5px padding, so any custom focus increase above 5 leaves part of the widened stroke unhoverable and unclickable.Suggested fix
if (renderMode > 0.0) { // Add 5 pixels padding for better hover detection - linkWidthPx += 5.0 / transformationMatrix[0][0]; + float hitPaddingPx = 5.0; + if (focusedLinkIndex == linkIndex) { + hitPaddingPx += max(0.0, focusedLinkWidthIncrease); + } + linkWidthPx += hitPaddingPx / 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, In the renderMode > 0.0 branch update linkWidthPx the same way as the visible pass: after the 5px padding add any hover/focus increases by checking hoveredLinkIndex == linkIndex and focusedLinkIndex == linkIndex and adding hoveredLinkWidthIncrease / transformationMatrix[0][0] and focusedLinkWidthIncrease / transformationMatrix[0][0] respectively (same symbols: renderMode, linkWidthPx, transformationMatrix, hoveredLinkIndex, focusedLinkIndex, hoveredLinkWidthIncrease, focusedLinkWidthIncrease, linkIndex) so the picking/hover pass accounts for custom focus/hover width increases.
🤖 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`:
- Line 359: The code is allocating linkStatusTexture eagerly via
updateLinkStatus() even when highlightedLinkIndices is undefined; change the
logic so updateLinkStatus either early-returns without creating a texture when
this.highlightedLinkIndices is falsy, and update callers (init/draw/hover and
any other places that call updateLinkStatus or check this.linkStatusTexture) to
guard the call with a check like if (!this.highlightedLinkIndices) return or
only call updateLinkStatus when this.highlightedLinkIndices is set; ensure
updateLinkStatus no longer builds the zero-filled rgba32float texture when
highlightedLinkIndices is undefined and that callers safely handle a missing
linkStatusTexture (skip binding/uploading) to avoid allocating GPU memory unless
highlighting is active.
---
Outside diff comments:
In `@src/modules/Lines/draw-curve-line.vert`:
- Around line 187-199: In the renderMode > 0.0 branch update linkWidthPx the
same way as the visible pass: after the 5px padding add any hover/focus
increases by checking hoveredLinkIndex == linkIndex and focusedLinkIndex ==
linkIndex and adding hoveredLinkWidthIncrease / transformationMatrix[0][0] and
focusedLinkWidthIncrease / transformationMatrix[0][0] respectively (same
symbols: renderMode, linkWidthPx, transformationMatrix, hoveredLinkIndex,
focusedLinkIndex, hoveredLinkWidthIncrease, focusedLinkWidthIncrease, linkIndex)
so the picking/hover pass accounts for custom focus/hover width increases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e28a224-6f70-4e93-a9ed-3b48e7af22e0
📒 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 (3)
- src/variables.ts
- src/modules/Points/find-points-in-polygon.frag
- src/modules/Points/index.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- src/helper.ts
- src/stories/clusters/polygon-selection/index.ts
- src/stories/shapes/image-example/index.ts
- README.md
- src/modules/Points/draw-highlighted.vert
- src/stories/2. configuration.mdx
- src/modules/Points/find-points-in-rect.frag
- migration-notes.md
- src/modules/Points/draw-points.frag
- src/modules/Points/find-hovered-point.vert
- src/stories/beginners/basic-set-up/index.ts
- src/modules/Points/draw-points.vert
Stukova
force-pushed
the
feat/config-driven-highlighting
branch
from
277bf12 to
e86ed22
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 (1)
src/modules/Lines/draw-curve-line.vert (1)
187-198:⚠️ Potential issue | 🟡 MinorKeep the focused stroke width in the picking mask.
focusedLinkWidthIncreaseis only applied in the normal render branch. If consumers raise it above the fixed 5px picking padding, the rendered focused link becomes wider than the hover/click mask and interactions can miss near the edge.🔧 Suggested adjustment
if (renderMode > 0.0) { - // Add 5 pixels padding for better hover detection - linkWidthPx += 5.0 / transformationMatrix[0][0]; + // Keep the picking mask at least as wide as the rendered focused stroke. + float pickPadding = 5.0; + if (focusedLinkIndex == linkIndex) { + pickPadding = max(pickPadding, focusedLinkWidthIncrease); + } + linkWidthPx += pickPadding / transformationMatrix[0][0]; } else { // Add pixel increase if this is the hovered link if (hoveredLinkIndex == linkIndex) { linkWidthPx += hoveredLinkWidthIncrease / transformationMatrix[0][0]; }🤖 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 - 198, The picking branch (when renderMode > 0.0) adds a fixed 5px padding to linkWidthPx but does not include focusedLinkWidthIncrease, so a focused link can be visually wider than its picking mask; update the conditional block that handles renderMode > 0.0 to also check if focusedLinkIndex == linkIndex and, if so, add focusedLinkWidthIncrease / transformationMatrix[0][0] to linkWidthPx (similar to how hoveredLinkIndex/focusedLinkIndex are handled in the else branch), ensuring the same coordinate-space scaling is applied.
🤖 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/index.ts`:
- Around line 1045-1059: In src/modules/Points/index.ts update the packing of
the status texture to validate indices properly: when iterating
highlightedPointIndices and outlinedPointIndices (used to write into state
buffer) only accept indices that are integers and within the actual point count
(use Number.isInteger(idx) && idx >= 0 && idx < data.pointsNumber) instead of
using the texture maxIndex; keep the existing R-channel initialization loop but
replace the current bounds check (idx < maxIndex) with this
integer+data.pointsNumber check before writing state[idx * 4] = 0 or state[idx *
4 + 1] = 1 so fractional or out-of-range indices are skipped.
---
Outside diff comments:
In `@src/modules/Lines/draw-curve-line.vert`:
- Around line 187-198: The picking branch (when renderMode > 0.0) adds a fixed
5px padding to linkWidthPx but does not include focusedLinkWidthIncrease, so a
focused link can be visually wider than its picking mask; update the conditional
block that handles renderMode > 0.0 to also check if focusedLinkIndex ==
linkIndex and, if so, add focusedLinkWidthIncrease / transformationMatrix[0][0]
to linkWidthPx (similar to how hoveredLinkIndex/focusedLinkIndex are handled in
the else branch), ensuring the same coordinate-space scaling is applied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8083ec50-696a-46fb-ace2-991eaf8bd88c
📒 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 (3)
- src/variables.ts
- README.md
- migration-notes.md
🚧 Files skipped from review as they are similar to previous changes (11)
- src/stories/clusters/polygon-selection/index.ts
- src/stories/shapes/image-example/index.ts
- src/modules/Points/find-points-in-polygon.frag
- src/helper.ts
- src/modules/Points/draw-highlighted.vert
- src/modules/Points/find-points-in-rect.frag
- src/modules/Store/index.ts
- src/stories/beginners/basic-set-up/index.ts
- src/modules/Points/draw-points.frag
- src/modules/Points/draw-points.vert
- src/index.ts
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Stukova
force-pushed
the
feat/config-driven-highlighting
branch
from
e86ed22 to
dd2dc37
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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)
src/modules/Lines/draw-curve-line.vert (1)
187-199:⚠️ Potential issue | 🟠 MajorKeep the pick buffer as wide as the rendered focused/hovered link.
The normal render path widens focused/hovered links, but the picking path falls back to a fixed 5px pad. When a link is both focused and hovered—or either width increase is configured above 5—the visible stroke extends outside the hover/click buffer.
🔧 Suggested fix
if (renderMode > 0.0) { - // Add 5 pixels padding for better hover detection - linkWidthPx += 5.0 / transformationMatrix[0][0]; + // Match the widened visible stroke and keep a small extra hit-area pad. + float hitPaddingPx = 5.0; + if (hoveredLinkIndex == linkIndex) { + hitPaddingPx += max(0.0, hoveredLinkWidthIncrease); + } + if (focusedLinkIndex == linkIndex) { + hitPaddingPx += max(0.0, focusedLinkWidthIncrease); + } + linkWidthPx += hitPaddingPx / transformationMatrix[0][0]; } else { // Add pixel increase if this is the hovered link if (hoveredLinkIndex == linkIndex) { linkWidthPx += hoveredLinkWidthIncrease / transformationMatrix[0][0]; }🤖 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 pick-path branch (renderMode > 0.0) currently applies a fixed 5px pad to linkWidthPx, which doesn't account for hovered/focused width increases; update the renderMode > 0.0 branch to compute the same additional width as the normal render path by checking hoveredLinkIndex == linkIndex and focusedLinkIndex == linkIndex and adding hoveredLinkWidthIncrease and focusedLinkWidthIncrease (both divided by transformationMatrix[0][0]) so the pick buffer matches the visible widened stroke; use the same symbols (renderMode, linkWidthPx, hoveredLinkIndex, focusedLinkIndex, linkIndex, hoveredLinkWidthIncrease, focusedLinkWidthIncrease, transformationMatrix) and sum increases (and the 5px minimum if desired) when computing linkWidthPx.src/config.ts (1)
37-57:⚠️ Potential issue | 🟠 MajorResolve the
pointGreyoutOpacitycontract before publishing this.This block currently describes two different behaviors: the
pointGreyoutColordocs saypointGreyoutOpacitymultiplies the final alpha, while thepointGreyoutOpacitydocs say it replacespointOpacityfor greyed-out points. Pick one contract and keepsrc/stories/2. configuration.mdxaligned, otherwise the v2→v3 migration story is ambiguous.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.ts` around lines 37 - 57, Choose a single contract: make pointGreyoutOpacity a multiplier applied to the final alpha of pointGreyoutColor (i.e., it multiplies the color's alpha after any computed darken/lighten), and update all docs to that behavior; update the JSDoc for pointGreyoutColor to explicitly say its alpha is combined with pointGreyoutOpacity (multiplied together), change the JSDoc for pointGreyoutOpacity to state it's a 0–1 multiplier applied to the final color alpha (not a replacement for pointOpacity), and make the same change in the v2→v3 migration story so both pointGreyoutColor and pointGreyoutOpacity are described consistently.
♻️ Duplicate comments (1)
src/modules/Points/index.ts (1)
1045-1057:⚠️ Potential issue | 🟡 MinorReject non-integer point indices before packing
pointStatusTexture.
highlightedPointIndices/outlinedPointIndicesare typed asnumber[], so a value like0.5currently writes into another RGBA channel instead of a real point slot. That silently drops or misroutes the requested status update.🔧 Suggested fix
if (hasHighlighting) { // Fill R channel with 1 (all greyed by default) for (let i = 0; i < state.length; i += 4) state[i] = 1 // Clear R channel for highlighted points for (const idx of highlightedPointIndices) { - if (idx >= 0 && idx < data.pointsNumber) state[idx * 4] = 0 + if (Number.isInteger(idx) && idx >= 0 && idx < data.pointsNumber) state[idx * 4] = 0 } } if (hasOutlining) { // Set G channel for outlined points for (const idx of outlinedPointIndices) { - if (idx >= 0 && idx < data.pointsNumber) state[idx * 4 + 1] = 1 + if (Number.isInteger(idx) && idx >= 0 && idx < data.pointsNumber) state[idx * 4 + 1] = 1 } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Points/index.ts` around lines 1045 - 1057, Reject non-integer indices before writing into the packed point status texture: in the loops that iterate highlightedPointIndices and outlinedPointIndices (where you write to state[idx*4] and state[idx*4+1]), add a guard to ensure idx is an integer (e.g., Number.isInteger(idx)) in addition to the existing bounds checks (idx >= 0 && idx < data.pointsNumber) and skip non-integer values so fractional indices cannot target the wrong RGBA channel when packing pointStatusTexture.
🧹 Nitpick comments (1)
src/index.ts (1)
1907-1917: Consider Set-based lookup for highlighted/outlined status in high-frequency hover detection.
Array.includes()is O(n) per call. Since hover detection runs frequently during mouse movement, this could become a bottleneck when many points are highlighted/outlined (e.g., thousands of indices).For typical use cases with small arrays, the current implementation is fine. If performance becomes a concern with large index arrays, consider maintaining
Setinstances in the store (updated when config changes) for O(1) lookup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 1907 - 1917, The hover handler currently uses Array.includes (this.config.highlightedPointIndices/outlinedPointIndices) which is O(n) per hover; change to O(1) lookups by adding Set instances (e.g., highlightedPointIndexSet, outlinedPointIndexSet) on the store and updating those Sets whenever the config arrays change (in the config setter/initializer paths), then replace includes calls in the hover path (the block using isMouseover and this.store.hoveredPoint) with Set.has checks to determine isHighlighted/isOutlined; ensure the Sets are kept in sync with the original arrays when config updates occur.
🤖 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 701-709: The loop un-greys links using highlightedLinkIndices but
allows fractional indices so writes land in wrong RGBA slots; update the check
in the highlightedLinkIndices loop (where state, linksNumber,
highlightedLinkIndices are used) to ensure each idx is an integer and in range —
e.g. replace the condition with Number.isInteger(idx) && idx >= 0 && idx <
linksNumber (or floor/round idx before multiplying) so you only write state[idx
* 4] for valid integer link slots.
---
Outside diff comments:
In `@src/config.ts`:
- Around line 37-57: Choose a single contract: make pointGreyoutOpacity a
multiplier applied to the final alpha of pointGreyoutColor (i.e., it multiplies
the color's alpha after any computed darken/lighten), and update all docs to
that behavior; update the JSDoc for pointGreyoutColor to explicitly say its
alpha is combined with pointGreyoutOpacity (multiplied together), change the
JSDoc for pointGreyoutOpacity to state it's a 0–1 multiplier applied to the
final color alpha (not a replacement for pointOpacity), and make the same change
in the v2→v3 migration story so both pointGreyoutColor and pointGreyoutOpacity
are described consistently.
In `@src/modules/Lines/draw-curve-line.vert`:
- Around line 187-199: The pick-path branch (renderMode > 0.0) currently applies
a fixed 5px pad to linkWidthPx, which doesn't account for hovered/focused width
increases; update the renderMode > 0.0 branch to compute the same additional
width as the normal render path by checking hoveredLinkIndex == linkIndex and
focusedLinkIndex == linkIndex and adding hoveredLinkWidthIncrease and
focusedLinkWidthIncrease (both divided by transformationMatrix[0][0]) so the
pick buffer matches the visible widened stroke; use the same symbols
(renderMode, linkWidthPx, hoveredLinkIndex, focusedLinkIndex, linkIndex,
hoveredLinkWidthIncrease, focusedLinkWidthIncrease, transformationMatrix) and
sum increases (and the 5px minimum if desired) when computing linkWidthPx.
---
Duplicate comments:
In `@src/modules/Points/index.ts`:
- Around line 1045-1057: Reject non-integer indices before writing into the
packed point status texture: in the loops that iterate highlightedPointIndices
and outlinedPointIndices (where you write to state[idx*4] and state[idx*4+1]),
add a guard to ensure idx is an integer (e.g., Number.isInteger(idx)) in
addition to the existing bounds checks (idx >= 0 && idx < data.pointsNumber) and
skip non-integer values so fractional indices cannot target the wrong RGBA
channel when packing pointStatusTexture.
---
Nitpick comments:
In `@src/index.ts`:
- Around line 1907-1917: The hover handler currently uses Array.includes
(this.config.highlightedPointIndices/outlinedPointIndices) which is O(n) per
hover; change to O(1) lookups by adding Set instances (e.g.,
highlightedPointIndexSet, outlinedPointIndexSet) on the store and updating those
Sets whenever the config arrays change (in the config setter/initializer paths),
then replace includes calls in the hover path (the block using isMouseover and
this.store.hoveredPoint) with Set.has checks to determine
isHighlighted/isOutlined; ensure the Sets are kept in sync with the original
arrays when config updates occur.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a37e9bf4-909a-4a93-a265-5a97bae846f3
📒 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 (3)
- src/helper.ts
- src/variables.ts
- README.md
🚧 Files skipped from review as they are similar to previous changes (10)
- src/modules/Points/draw-highlighted.vert
- src/stories/shapes/image-example/index.ts
- src/modules/Points/find-points-in-polygon.frag
- src/modules/Points/find-hovered-point.vert
- src/modules/Points/draw-points.frag
- src/stories/clusters/polygon-selection/index.ts
- src/modules/Points/find-points-in-rect.frag
- src/modules/Points/draw-points.vert
- migration-notes.md
- src/stories/beginners/basic-set-up/index.ts
Comment on lines
+701
to
+709
| // Mark all links as greyed out (R=1) | ||
| for (let i = 0; i < linksNumber; i++) { | ||
| state[i * 4] = 1 | ||
| } | ||
| // Un-grey highlighted links | ||
| for (const idx of highlightedLinkIndices) { | ||
| if (idx >= 0 && idx < linksNumber) { | ||
| state[idx * 4] = 0 | ||
| } |
There was a problem hiding this comment.
Validate highlightedLinkIndices as integers before writing the status texture.
The current range check accepts fractional values, and those writes land in arbitrary RGBA channels rather than the intended link slot. The requested link then never gets un-greyed.
🔧 Suggested fix
// Un-grey highlighted links
for (const idx of highlightedLinkIndices) {
- if (idx >= 0 && idx < linksNumber) {
+ if (Number.isInteger(idx) && idx >= 0 && idx < linksNumber) {
state[idx * 4] = 0
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Mark all links as greyed out (R=1) | |
| for (let i = 0; i < linksNumber; i++) { | |
| state[i * 4] = 1 | |
| } | |
| // Un-grey highlighted links | |
| for (const idx of highlightedLinkIndices) { | |
| if (idx >= 0 && idx < linksNumber) { | |
| state[idx * 4] = 0 | |
| } | |
| // Mark all links as greyed out (R=1) | |
| for (let i = 0; i < linksNumber; i++) { | |
| state[i * 4] = 1 | |
| } | |
| // Un-grey highlighted links | |
| for (const idx of highlightedLinkIndices) { | |
| if (Number.isInteger(idx) && idx >= 0 && idx < linksNumber) { | |
| state[idx * 4] = 0 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/Lines/index.ts` around lines 701 - 709, The loop un-greys links
using highlightedLinkIndices but allows fractional indices so writes land in
wrong RGBA slots; update the check in the highlightedLinkIndices loop (where
state, linksNumber, highlightedLinkIndices are used) to ensure each idx is an
integer and in range — e.g. replace the condition with Number.isInteger(idx) &&
idx >= 0 && idx < linksNumber (or floor/round idx before multiplying) so you
only write state[idx * 4] for valid integer link slots.
dd2dc37 to
778b87d
Compare
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
…nly fields Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
…ence 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>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
…date Bumps the npm_and_yarn group with 1 update in the / directory: [picomatch](https://github.com/micromatch/picomatch). Updates `picomatch` from 4.0.2 to 4.0.4 - [Release notes](https://github.com/micromatch/picomatch/releases) - [Changelog](https://github.com/micromatch/picomatch/blob/master/CHANGELOG.md) - [Commits](micromatch/picomatch@4.0.2...4.0.4) Updates `picomatch` from 2.3.1 to 2.3.2 - [Release notes](https://github.com/micromatch/picomatch/releases) - [Changelog](https://github.com/micromatch/picomatch/blob/master/CHANGELOG.md) - [Commits](micromatch/picomatch@4.0.2...4.0.4) --- updated-dependencies: - dependency-name: picomatch dependency-version: 4.0.4 dependency-type: indirect dependency-group: npm_and_yarn - dependency-name: picomatch dependency-version: 2.3.2 dependency-type: indirect dependency-group: npm_and_yarn ... Signed-off-by: dependabot[bot] <support@github.com>
…g, outlining, and focusing Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Stukova
force-pushed
the
feat/config-driven-highlighting
branch
from
778b87d to
7d5b2eb
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.
Summary
Replace the imperative selection API with config-driven properties for controlling point and link visual states. Point
and link highlighting are independent.
Removed methods
selectPointByIndex()/selectPointsByIndices()setConfigPartial({ highlightedPointIndices })selectPointsInRect()findPointsInRect()+setConfigPartial({ highlightedPointIndices })selectPointsInPolygon()findPointsInPolygon()+setConfigPartial({ highlightedPointIndices })unselectPoints()setConfigPartial({ highlightedPointIndices: undefined, highlightedLinkIndices: undefined })getSelectedIndices()getPointsInRect()/getPointsInPolygon()findPointsInRect()/findPointsInPolygon()getAdjacentIndices(index)getNeighboringPointIndices(pointIndices)New config properties
highlightedPointIndicesnumber[] | undefinedundefinedoutlinedPointIndicesnumber[] | undefinedundefinedoutlinedPointRingColorstring | RGBA'white'highlightedLinkIndicesnumber[] | undefinedundefinedfocusedLinkIndexnumber | undefinedundefinedfocusedLinkWidthIncreasenumber5New / updated methods
findPointsInRect(rect)number[]findPointsInPolygon(path)number[]getNeighboringPointIndices(pointIndices)number[]number | number[])getConnectedLinkIndices(pointIndices)number[]number | number[])getConnectedPointIndices(linkIndices)number[]number | number[])Callback changes
onPointMouseOverisSelected→isHighlighted; new 5th paramisOutlinedSummary by CodeRabbit