Add Link Hover Detection with Mouse Event Callbacks#170
Conversation
WalkthroughAdds GPU-based link picking and full link-hover/click support: new shaders, FBOs, buffers, and draw commands in Lines; Graph hover orchestration extended to points+links with throttling and cursors; Store/config expose link-hover state, colors, cursors and callbacks; docs, examples, and minor renames/defaults updated. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Graph
participant Lines
participant Shaders
participant Store
participant Config
User->>Graph: move mouse / click
Graph->>Graph: frame() (throttled by MAX_HOVER_DETECTION_DELAY)
alt detection executed
Graph->>Lines: render links (renderMode=1.0) -> linkIndexFbo
Note right of Lines #E6F2FF: index-render pass writes per-link indices
Lines->>Shaders: fullscreen hovered-line-index pass (sample linkIndexFbo at mouse)
Shaders-->>Lines: hovered link index (or -1)
Lines-->>Graph: hovered link index
Graph->>Store: set hoveredLinkIndex / setHoveredLinkColor
alt hovered link changed
Graph->>Config: onLinkMouseOut()
Graph->>Config: onLinkMouseOver(linkIndex)
end
Graph->>Graph: updateCanvasCursor() (apply hoveredLinkCursor if link hovered)
end
User->>Graph: click
alt point hovered
Graph->>Config: onPointClick(index, position, event)
else link hovered
Graph->>Config: onLinkClick(linkIndex, event)
else background
Graph->>Config: onBackgroundClick(event)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
✨ Finishing touches🧪 Generate unit tests
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: 2
🔭 Outside diff range comments (1)
src/modules/Lines/index.ts (1)
10-259: Consider adding a cleanup method for WebGL resourcesThe class creates several WebGL resources (framebuffers, buffers) but doesn't provide a way to clean them up. Consider adding a destroy method to prevent potential memory leaks.
Add a cleanup method:
public destroy(): void { // Destroy framebuffers this.linkIndexFbo?.destroy?.() this.hoveredLineIndexFbo?.destroy?.() // Destroy buffers this.pointsBuffer?.destroy?.() this.colorBuffer?.destroy?.() this.widthBuffer?.destroy?.() this.arrowBuffer?.destroy?.() this.curveLineBuffer?.destroy?.() this.linkIndexBuffer?.destroy?.() this.quadBuffer?.destroy?.() // Clear references this.linkIndexFbo = undefined this.hoveredLineIndexFbo = undefined this.drawCurveCommand = undefined this.hoveredLineIndexCommand = undefined this.pointsBuffer = undefined this.colorBuffer = undefined this.widthBuffer = undefined this.arrowBuffer = undefined this.curveLineBuffer = undefined this.linkIndexBuffer = undefined this.quadBuffer = undefined }Then call this method from the parent Graph's destroy method.
🧹 Nitpick comments (3)
src/modules/core-module.ts (1)
13-13: Remove or document the debug property.This debug property appears unrelated to the PR's link hover detection objective and may be leftover development code. Consider removing it or documenting its purpose if it's intentionally included.
- public _debugRandomNumber = Math.floor(Math.random() * 1000)src/modules/Lines/draw-curve-line.vert (1)
129-131: Consider making the hover detection width increase configurableThe hardcoded value of 5.0 for increasing link width during hover detection could be made configurable for better flexibility.
Consider adding a uniform for this value:
uniform float renderMode; uniform float hoveredLinkIndex; +uniform float hoverDetectionWidthIncrease; // ... later in the code if (renderMode > 0.0) { - linkWidth += 5.0; + linkWidth += hoverDetectionWidthIncrease; }src/modules/Lines/index.ts (1)
176-190: Optimize framebuffer initializationThe framebuffer is created without parameters and then immediately reconfigured. This can be optimized by creating it with the texture configuration directly.
public updateLinkIndexFbo (): void { const { reglInstance, store } = this - if (!this.linkIndexFbo) this.linkIndexFbo = reglInstance.framebuffer() - this.linkIndexFbo({ - color: reglInstance.texture({ - width: store.screenSize[0], - height: store.screenSize[1], - format: 'rgba', - type: 'float', - }), - depth: false, - stencil: false, - }) + if (!this.linkIndexFbo) { + this.linkIndexFbo = reglInstance.framebuffer({ + color: reglInstance.texture({ + width: store.screenSize[0], + height: store.screenSize[1], + format: 'rgba', + type: 'float', + }), + depth: false, + stencil: false, + }) + } else { + this.linkIndexFbo({ + color: reglInstance.texture({ + width: store.screenSize[0], + height: store.screenSize[1], + format: 'rgba', + type: 'float', + }), + depth: false, + stencil: false, + }) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/config.ts(2 hunks)src/index.ts(8 hunks)src/modules/Lines/draw-curve-line.frag(2 hunks)src/modules/Lines/draw-curve-line.vert(5 hunks)src/modules/Lines/hovered-line-index.frag(1 hunks)src/modules/Lines/hovered-line-index.vert(1 hunks)src/modules/Lines/index.ts(7 hunks)src/modules/Store/index.ts(1 hunks)src/modules/core-module.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: buffer creation in regl (webgl) contexts using `reglinstance.buffer()` is treated as a reliable oper...
Learnt from: Stukova
PR: cosmosgl/graph#169
File: src/modules/Points/index.ts:555-560
Timestamp: 2025-07-31T06:19:00.654Z
Learning: Buffer creation in regl (WebGL) contexts using `reglInstance.buffer()` is treated as a reliable operation in this codebase. The established pattern across all modules is to create buffers without null checks after creation, trusting that if buffer creation fails, it indicates more fundamental WebGL context issues that should bubble up rather than be handled silently.
Applied to files:
src/modules/Lines/index.ts
🧬 Code Graph Analysis (2)
src/config.ts (1)
src/modules/GraphData/index.ts (1)
GraphData(15-321)
src/modules/Lines/index.ts (2)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Points/index.ts (1)
Points(24-826)
🔇 Additional comments (7)
src/modules/Store/index.ts (1)
29-29: LGTM!The
hoveredLinkIndexproperty addition is well-integrated, follows existing naming conventions, and properly supports the link hover detection feature.src/modules/Lines/hovered-line-index.vert (1)
1-8: LGTM!The vertex shader correctly implements a standard fullscreen quad with proper texture coordinate mapping from clip space [-1,1] to texture space [0,1].
src/modules/Lines/hovered-line-index.frag (1)
9-27: LGTM!The fragment shader correctly implements GPU-based link picking by sampling the link index texture at the mouse position and validating the result using both alpha channel and index value checks.
src/modules/Lines/draw-curve-line.frag (2)
9-12: LGTM!The new varying
linkIndexand uniformrenderModeare properly declared and enable dual-mode rendering for both normal display and GPU-based picking.
38-45: LGTM!The conditional rendering logic correctly implements dual-mode behavior: outputting encoded link indices for picking mode while preserving original rendering for normal mode. The fragment visibility check ensures accurate picking results.
src/index.ts (2)
1414-1442: LGTM! Well-structured hover detection for linksThe implementation correctly:
- Skips link hover detection when a point is already hovered (points have priority)
- Properly manages state transitions with mouseOver/mouseOut callbacks
- Handles edge cases like undefined hoveredLinkIndex
1272-1274: Good defensive programming for frame recursionThe conditional recursive call prevents potential issues after destruction.
Stukova
force-pushed
the
feature/link-hover-detection
branch
from
0d750b4 to
b0106d0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
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/index.ts (1)
117-129: Disable blending during index-picking pass to avoid ambiguityThe same draw command is reused for normal and picking passes, but blending stays enabled. If the picking fragment shader ever outputs alpha < 1.0, overlapping links will blend and corrupt indices. Safer to disable blending in the picking pass.
Apply this minimal change to make blending a dynamic prop:
blend: { - enable: true, + enable: reglInstance.prop<{ blendEnable: boolean }, 'blendEnable'>('blendEnable'), func: { dstRGB: 'one minus src alpha', srcRGB: 'src alpha', dstAlpha: 'one minus src alpha', srcAlpha: 'one', }, equation: { rgb: 'add', alpha: 'add', }, },Then pass the prop in the two call sites:
- this.drawCurveCommand?.({ framebuffer: null, renderMode: 0.0 }) + this.drawCurveCommand?.({ framebuffer: null, renderMode: 0.0, blendEnable: true })- this.drawCurveCommand?.({ framebuffer: this.linkIndexFbo, renderMode: 1.0 }) + this.drawCurveCommand?.({ framebuffer: this.linkIndexFbo, renderMode: 1.0, blendEnable: false })Also applies to: 174-176, 256-256
♻️ Duplicate comments (2)
src/config.ts (1)
395-409: FixonLinkMouseOutsignature/doc mismatch (repeat)Docs say the callback receives
(linkIndex: number), but the type is() => void. Please align the type with the docs (or vice versa). This was flagged previously and remains unresolved.Apply this diff:
/** * Callback function that will be called when the mouse moves out of a link. - * The link index will be passed as the first argument: - * `(linkIndex: number) => void`. + * The link index will be passed as the first argument: + * `(linkIndex: number) => void`. * Default value: `undefined` */ - onLinkMouseOut?: () => void; + onLinkMouseOut?: (linkIndex: number) => void;Optionally consider parity with point callbacks by including the event and possibly mouse position in both
onLinkMouseOverandonLinkMouseOut. That can be a follow-up if desired.src/modules/Lines/index.ts (1)
248-260: Hover resolve pipeline — add failure logging and ensure CPU readback
If FBO creation fails, the method silently returns after
updateLinkIndexFbo()without feedback. Log and bail explicitly. This was suggested earlier; re-upping here.After running
hoveredLineIndexCommand, ensure there is a CPU readback that updatesstore.hoveredLinkIndex. It’s not shown here; if it happens in another module (e.g., src/index.ts), we’re good. Otherwise, addregl.readfromhoveredLineIndexFboand decode the index.Would you like me to wire a tiny readback helper that decodes the 1x1 RGBA float/uint8 into the link index and updates the Store?
🧹 Nitpick comments (5)
src/variables.ts (1)
30-31: Defaults for hovered link visuals added — consider guarding against negative widthThe new
hoveredLinkColorandhoveredLinkWidthIncreasedefaults look good. Tiny safety nit: if users pass a negativehoveredLinkWidthIncrease, the shader will thin hovered links. If that’s not intended, clamp to 0 in the shader (see my comment in draw-curve-line.vert).src/modules/Lines/draw-curve-line.vert (3)
136-144: Picking padding and hover width: clamp width increase and avoid magic number
- Add a named constant for the 5px picking padding.
- Prevent negative
hoveredLinkWidthIncreasefrom shrinking the line on hover.Apply within-range changes:
- if (renderMode > 0.0) { - // Add 5 pixels padding for better hover detection - linkWidthPx += 5.0 / transformationMatrix[0][0]; + if (renderMode > 0.0) { + // Add padding (in screen px) for better hover detection + linkWidthPx += PICK_PADDING_PX / transformationMatrix[0][0]; } else { // Add pixel increase if this is the hovered link - if (hoveredLinkIndex == linkIndex) { - linkWidthPx += hoveredLinkWidthIncrease / transformationMatrix[0][0]; + if (hoveredLinkIndex == linkIndex) { + linkWidthPx += max(0.0, hoveredLinkWidthIncrease) / transformationMatrix[0][0]; } }And add the constant once near the top (outside the selected range):
const float PICK_PADDING_PX = 5.0;
164-167: Hover color override ignores global opacity/greyout — multiply alpha instead of replacingRight now
rgbaColor = hoveredLinkColor;discards the computed opacity (distance fade, global linkOpacity, greyout). To keep highlight color while preserving scene-wide transparency effects, modulate alpha:- if (hoveredLinkIndex == linkIndex) { - rgbaColor = hoveredLinkColor; - } + if (hoveredLinkIndex == linkIndex) { + // Preserve computed opacity (distance, greyout, global opacity) while applying hover RGB + rgbaColor = vec4(hoveredLinkColor.rgb, hoveredLinkColor.a * rgbaColor.a); + }
27-31: Float equality on indices — low risk but document constraintsComparing floats
hoveredLinkIndex == linkIndexis fine as long as link counts stay well below ~16M so indices are exactly representable. Given typical graph sizes this is OK. If you ever push huge link counts, considerfloor(linkIndex + 0.5)before comparison to counter interpolation rounding.Also applies to: 83-84
src/modules/Lines/index.ts (1)
178-192: FBO reallocation on every call — optionally skip when size unchanged
updateLinkIndexFbo()always reallocates. If it’s called from resize only, that’s fine. If called more frequently, consider a guard to skip whenstore.screenSizedidn’t change to reduce GC pressure.Happy to draft a tiny size-check if you want it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
src/config.ts(6 hunks)src/index.ts(9 hunks)src/modules/Lines/draw-curve-line.frag(2 hunks)src/modules/Lines/draw-curve-line.vert(5 hunks)src/modules/Lines/hovered-line-index.frag(1 hunks)src/modules/Lines/hovered-line-index.vert(1 hunks)src/modules/Lines/index.ts(7 hunks)src/modules/Store/index.ts(2 hunks)src/modules/core-module.ts(1 hunks)src/variables.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/modules/Lines/draw-curve-line.frag
- src/modules/Store/index.ts
- src/modules/core-module.ts
- src/modules/Lines/hovered-line-index.vert
- src/modules/Lines/hovered-line-index.frag
- src/index.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-31T06:19:00.654Z
Learnt from: Stukova
PR: cosmosgl/graph#169
File: src/modules/Points/index.ts:555-560
Timestamp: 2025-07-31T06:19:00.654Z
Learning: Buffer creation in regl (WebGL) contexts using `reglInstance.buffer()` is treated as a reliable operation in this codebase. The established pattern across all modules is to create buffers without null checks after creation, trusting that if buffer creation fails, it indicates more fundamental WebGL context issues that should bubble up rather than be handled silently.
Applied to files:
src/modules/Lines/index.ts
🧬 Code graph analysis (2)
src/config.ts (1)
src/variables.ts (1)
defaultConfigValues(12-64)
src/modules/Lines/index.ts (2)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Points/index.ts (1)
Points(24-826)
🔇 Additional comments (9)
src/variables.ts (1)
27-27: Add hovered link cursor default — looks consistentThe new
hoveredLinkCursor: 'auto'mirrors the existing point-hover cursor default and integrates cleanly with cursor updates elsewhere. No issues.src/config.ts (3)
100-105: New hoveredLinkCursor option — docs and type alignDocs, type, and defaults line up with variables.ts. Looks good.
166-178: Hovered link styling options — aligned across modules
hoveredLinkColorandhoveredLinkWidthIncreaseare clearly documented and match Store/Lines usage. No issues.
582-595: Config wires new link-hover fields — consistent with defaults
hoveredLinkCursor,hoveredLinkColor,hoveredLinkWidthIncrease, and hover callbacks are exposed with correct defaults. Looks consistent with variables.ts and Lines module usage.Also applies to: 631-633
src/modules/Lines/index.ts (5)
27-42: Float FBOs for picking — verify extension support and fallback
linkIndexFboandhoveredLineIndexFbousetype: 'float'. On WebGL1 this requires OES_texture_float and WEBGL_color_buffer_float; on some platforms only half-floats are renderable. Make sure the capability check exists upstream, or provide a graceful fallback touint8or half-float when float color buffers aren’t supported.Would you like a small capability probe + fallback snippet for your initialization path?
83-89: Per-link index attribute wiring — LGTMAdding
linkIndiceswith proper divisor/stride matches the vertex shader and the buffer layout built inupdatePointsBuffer. Good.
108-112: Hover-related uniforms bound correctly
hoveredLinkIndex,hoveredLinkColor,hoveredLinkWidthIncrease, andrenderModeare passed as expected. Using-1sentinel for “no hover” is clear.
194-221: Indices buffer generation — LGTMSequential indices per link are correct for the picking path and match the shader’s expectation.
141-159: Confirm codebase convention for FBO texture samplingI didn’t find any other instances of sampling an FBO in a uniform across the repo, so there isn’t an established pattern yet. regl will accept a framebuffer object and implicitly use its first color attachment, but it’s often clearer to reference the texture directly via
fbo.color[0]. Please choose one of the following for consistency and apply it everywhere:• In src/modules/Lines/index.ts (lines 141–159), replace:
linkIndexTexture: () => this.linkIndexFbowith
linkIndexTexture: () => this.linkIndexFbo.color[0]if you decide to always pass textures explicitly.
• Alternatively, if you prefer passing the framebuffer object itself, add a brief comment here (and at any future FBO uniform) explaining that regl will use the first color attachment automatically.
Once you’ve confirmed the convention, update this call—and any other FBO-based uniforms—to match.
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
…ease` config props Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Stukova
force-pushed
the
feature/link-hover-detection
branch
from
b0106d0 to
0124281
Compare
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
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/index.ts (1)
109-110: Add WebGL color buffer float extensions to regl initializationThe application creates numerous float‐typed framebuffers across many modules (Lines, Points, ForceManyBody, Clusters, etc.), all relying on rendering to floating‐point attachments. To ensure these render targets work reliably on platforms that require explicit color buffer float support, update the regl constructor in src/index.ts (around lines 109–110) to request the necessary extensions:
• File: src/index.ts
Location: inside the regl initialization object (currently at lines 109–110)- extensions: ['OES_texture_float', 'ANGLE_instanced_arrays'], + extensions: [ + 'OES_texture_float', + 'WEBGL_color_buffer_float', // required for float FBO rendering on WebGL1 + 'ANGLE_instanced_arrays' + ], + optionalExtensions: [ + 'EXT_color_buffer_float', // WebGL2 equivalent + 'OES_texture_half_float', // half-float fallback + 'EXT_color_buffer_half_float' + ],Once this is in place, all subsequent calls to
reglInstance.framebuffer({ type: 'float', … })across the codebase will be covered by the requested extensions.
♻️ Duplicate comments (1)
src/modules/Lines/index.ts (1)
248-260: Add guards and error logging to avoid silent failures in picking path
- If buffers/commands aren’t ready, drawCurveCommand may no-op or error.
- If linkIndexFbo creation failed, we should not proceed silently.
- This aligns with prior feedback.
public findHoveredLine (): void { if (!this.data.linksNumber) return - if (!this.linkIndexFbo) this.updateLinkIndexFbo() + if (!this.linkIndexFbo) this.updateLinkIndexFbo() + if (!this.linkIndexFbo || !this.drawCurveCommand || !this.hoveredLineIndexCommand || + !this.pointsBuffer || !this.colorBuffer || !this.widthBuffer || !this.arrowBuffer || + !this.curveLineBuffer) { + console.warn('[Lines.findHoveredLine] Picking prerequisites not initialized') + return + } this.reglInstance.clear({ framebuffer: this.linkIndexFbo as regl.Framebuffer2D, color: [0, 0, 0, 0], }) // Render to index buffer for picking/hover detection (renderMode: 1.0 = index rendering) this.drawCurveCommand?.({ framebuffer: this.linkIndexFbo, renderMode: 1.0 }) // Execute the command to read the link index at mouse position this.hoveredLineIndexCommand?.() }
🧹 Nitpick comments (7)
src/modules/Lines/draw-curve-line.frag (1)
38-45: Eliminate unnecessary write on “no-hit” path to reduce overdrawIn the picking pass, the else branch writes a sentinel color but with alpha 0.0. Because blending is enabled and srcAlpha=0, this write is effectively a no-op. You can discard or skip the write to reduce fragment work (and avoid the negative sentinel altogether).
Apply either of the following diffs (pick one):
Option A: discard fragments with zero opacity
- if (renderMode > 0.0) { - if (opacity > 0.0) { - gl_FragColor = vec4(linkIndex, 0.0, 0.0, 1.0); - } else { - gl_FragColor = vec4(-1.0, 0.0, 0.0, 0.0); - } - } else gl_FragColor = vec4(color, opacity); + if (renderMode > 0.0) { + if (opacity > 0.0) { + gl_FragColor = vec4(linkIndex, 0.0, 0.0, 1.0); + } else { + discard; + } + } else gl_FragColor = vec4(color, opacity);Option B: write transparent black for “no-hit”
- } else { - gl_FragColor = vec4(-1.0, 0.0, 0.0, 0.0); - } + } else { + gl_FragColor = vec4(0.0); + }src/index.ts (2)
1429-1439: Centralized hover orchestration is clean; consider honoring renderLinksfindHoveredItem centralizes point+link hover and cursor updates. Consider also checking config.renderLinks before findHoveredLine to avoid firing link hover callbacks when links are not rendered.
- this.findHoveredPoint() - if (this.graph.linksNumber) this.findHoveredLine() + this.findHoveredPoint() + if (this.graph.linksNumber && this.config.renderLinks) this.findHoveredLine()
124-127: Clear hover state on mouseleave to avoid stale highlightsWhen the cursor leaves the canvas, hover detection stops (by design), but the last hovered point/link remains set until re-entry. Clear both states and fire corresponding “out” callbacks on mouseleave to keep UI consistent.
- .on('mouseleave.cosmos', () => { this._isMouseOnCanvas = false }) + .on('mouseleave.cosmos', () => { + this._isMouseOnCanvas = false + const hadPoint = !!this.store.hoveredPoint + const hadLink = this.store.hoveredLinkIndex !== undefined + this.store.hoveredPoint = undefined + this.store.hoveredLinkIndex = undefined + if (hadPoint) this.config.onPointMouseOut?.(this.currentEvent) + if (hadLink) this.config.onLinkMouseOut?.() + this.updateCanvasCursor() + })src/modules/Lines/index.ts (4)
11-12: Public FBOs for picking are fine, but ensure lifecycle managementExposing linkIndexFbo/hoveredLineIndexFbo is practical for interop with Graph. Just ensure they’re recreated on resize (linkIndexFbo already is) and destroyed on module teardown (if/when a destroy() exists).
27-42: Initialize picking FBOs early; also guard against zero-sized canvasesCalling updateLinkIndexFbo() in initPrograms keeps the picking buffer in sync. Add a minimal size guard so creation doesn’t fail when the canvas is 0×0 (e.g., hidden or not yet laid out).
public updateLinkIndexFbo (): void { const { reglInstance, store } = this - - if (!this.linkIndexFbo) this.linkIndexFbo = reglInstance.framebuffer() - this.linkIndexFbo({ + const w = Math.max(1, store.screenSize[0] || 0) + const h = Math.max(1, store.screenSize[1] || 0) + if (!this.linkIndexFbo) this.linkIndexFbo = reglInstance.framebuffer() + this.linkIndexFbo({ color: reglInstance.texture({ - width: store.screenSize[0], - height: store.screenSize[1], + width: w, + height: h, format: 'rgba', type: 'float', }), depth: false, stencil: false, }) }
178-192: Float render target creation: consider explicit extension check/loggingSince this FBO uses type 'float', add a one-time capability check (WEBGL_color_buffer_float / EXT_color_buffer_float) and log a warning if unsupported to help users debug black/empty picking buffers on some GPUs.
I can add a small guard that inspects regl.limits and tries to enable the extension, logging a clear warning if unavailable.
248-260: Consider limiting the picking render to a scissored region around the cursorRendering the entire screen every (third) frame can be costly on large canvases. Using a small scissor box around the mouse for the first pass could significantly reduce fill cost.
I can prototype a scissored picking path using regl.scissor with a configurable radius around the cursor.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
src/config.ts(6 hunks)src/index.ts(9 hunks)src/modules/Lines/draw-curve-line.frag(2 hunks)src/modules/Lines/draw-curve-line.vert(5 hunks)src/modules/Lines/hovered-line-index.frag(1 hunks)src/modules/Lines/hovered-line-index.vert(1 hunks)src/modules/Lines/index.ts(7 hunks)src/modules/Store/index.ts(2 hunks)src/modules/core-module.ts(1 hunks)src/variables.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/modules/core-module.ts
- src/modules/Store/index.ts
- src/variables.ts
- src/modules/Lines/hovered-line-index.vert
- src/modules/Lines/hovered-line-index.frag
- src/modules/Lines/draw-curve-line.vert
- src/config.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-31T06:19:00.654Z
Learnt from: Stukova
PR: cosmosgl/graph#169
File: src/modules/Points/index.ts:555-560
Timestamp: 2025-07-31T06:19:00.654Z
Learning: Buffer creation in regl (WebGL) contexts using `reglInstance.buffer()` is treated as a reliable operation in this codebase. The established pattern across all modules is to create buffers without null checks after creation, trusting that if buffer creation fails, it indicates more fundamental WebGL context issues that should bubble up rather than be handled silently.
Applied to files:
src/modules/Lines/index.ts
🧬 Code graph analysis (2)
src/modules/Lines/index.ts (2)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Points/index.ts (1)
Points(25-934)
src/index.ts (4)
src/helper.ts (1)
readPixels(38-45)src/modules/Points/index.ts (1)
findHoveredPoint(723-726)src/modules/Drag/index.ts (1)
Drag(5-33)src/modules/GraphData/index.ts (1)
GraphData(16-383)
🔇 Additional comments (16)
src/modules/Lines/draw-curve-line.frag (1)
9-12: Per-instance link index plumbing looks correctPassing linkIndex via an instanced attribute and varying is appropriate for the picking pass. No issues here.
src/index.ts (10)
53-60: Frame-throttled hover execution: good guardrailsThe new _findHoveredItemExecutionCount + _isMouseOnCanvas guards provide a simple, low-cost throttle. Looks good.
191-193: Propagate hoveredLinkColor from config to storeGood addition; keeps configuration reactive.
267-269: React to hoveredLinkColor changes in setConfigChange detection + propagation is consistent with other config fields.
1254-1254: Early exit in frame() when destroyedPrevents scheduling work after teardown.
1262-1265: Skip hover work while draggingAvoids extra readPixels during drag; good UX/perf tradeoff.
1330-1333: Destroy guard before scheduling next framePrevents runaway RAF loops post-destroy.
1379-1379: Resize guard when destroyedSafe-guard for resize path.
1397-1397: Update link index FBO on resizeKey to keep picking buffer in sync with the canvas. Good catch.
1442-1470: Point hover flow is soundClear separation of over/out paths; correct use of readPixels; events fire only on changes.
1503-1511: Cursor update considers link hoverNicely integrates hoveredLinkCursor without interfering with drag/point logic.
src/modules/Lines/index.ts (5)
83-89: Instanced linkIndices attribute is correctPer-instance sequential indices are the right approach for constant link IDs across all fragments of an instance.
108-112: Hover uniforms and renderMode prop are well-wiredhoveredLinkIndex/Color/WidthIncrease + renderMode prop give the shaders what they need for both normal and picking passes.
141-159: Fullscreen quad setup for second pass is correctMinimal quad buffer, triangle strip, and 1×1 FBO target: all good.
174-176: Explicit normal pass call via props is clearPassing framebuffer: null, renderMode: 0.0 keeps the dual-use draw command simple.
215-221: Sequential link index buffer generation is fineStraightforward and matches instancing.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/index.ts (1)
1472-1500: Bug: mouseout check fails for link index 0 (falsy check) — fix to explicit undefinedIf the previously hovered link index is 0,
if (this.store.hoveredLinkIndex)is falsy, soonLinkMouseOutwon’t fire and the hover state can remain stale on exit.Apply this minimal fix:
- } else { - if (this.store.hoveredLinkIndex) isMouseout = true - this.store.hoveredLinkIndex = undefined - } + } else { + if (this.store.hoveredLinkIndex !== undefined) isMouseout = true + this.store.hoveredLinkIndex = undefined + }To catch similar cases elsewhere, run:
#!/bin/bash # Find any truthy checks on hoveredLinkIndex that should be explicit undefined checks rg -nP --type=ts -C2 '\bif\s*\(\s*this\.store\.hoveredLinkIndex\s*\)'
🧹 Nitpick comments (2)
src/index.ts (2)
1429-1439: Optional: make hover throttle frame-rate independentThe frame-count-based throttle can feel different across devices. A time-based throttle (e.g., ≥ X ms since last check) yields consistent UX.
Example (within this method; you’d add a private
_lastHoverCheckTs = 0field):- if (this._findHoveredItemExecutionCount < MAX_HOVER_DETECTION_DELAY) { - this._findHoveredItemExecutionCount += 1 - return - } - this._findHoveredItemExecutionCount = 0 + const now = performance.now() + if (now - this._lastHoverCheckTs < MAX_HOVER_DETECTION_DELAY) return + this._lastHoverCheckTs = now
1472-1500: API parity: consider passing event to link callbacks
onPointMouseOver/Outreceive the current event, butonLinkMouseOver/Outdo not. For API consistency (and downstream handlers that might need modifier keys), consider optionally passingthis.currentEvent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/index.ts(10 hunks)src/modules/Store/index.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/modules/Store/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/index.ts (4)
src/modules/Store/index.ts (2)
ALPHA_MIN(7-7)MAX_HOVER_DETECTION_DELAY(15-15)src/helper.ts (1)
readPixels(38-45)src/modules/Points/index.ts (1)
findHoveredPoint(723-726)src/modules/GraphData/index.ts (1)
GraphData(16-383)
🔇 Additional comments (5)
src/index.ts (5)
20-20: Importing MAX_HOVER_DETECTION_DELAY and Hovered type — good expansionThe new imports are appropriate and used correctly by the hover throttle and event typing. No issues.
1254-1265: Frame-loop destruction guard and non-drag hover checks — solidEarly return on destroy and the re-entrancy guard before scheduling the next frame are good. Deferring hover detection while dragging reduces GPU readbacks during drag, which is desirable.
Also applies to: 1330-1333
1379-1379: Safe resize + link index FBO refresh — good; keep it resize-scopedThe extra destroyed guard and
lines?.updateLinkIndexFbo()on actual size changes are appropriate and avoid redundant work each frame.Also applies to: 1397-1398
1441-1471: Point hover detection — looks correctReads from the hovered FBO, updates store, and emits over/out callbacks with the current event. No issues spotted.
1503-1510: Cursor updates for link hover — LGTMCursor priority order (dragging > point > link > default) is sensible and matches expected UX.
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/variables.ts (1)
17-18: Fix config property mismatch for link arrow sizingThe default config key in src/variables.ts (
arrowSizeScale) does not align with the consumed property name (linkArrowsSizeScale) used in src/config.ts and throughout the Lines module. This leads to:
- Docs and the Config interface referring to
linkArrowsSizeScale(see src/config.ts and src/stories/2.configuration.mdx).- The default config value only defined as
arrowSizeScalein src/variables.ts.- src/config.ts initializing
instead of pulling from a matching key.public linkArrowsSizeScale = defaultConfigValues.arrowSizeScaleTo resolve and preserve backward compatibility, update both defaults and mapping:
• src/variables.ts
pointSizeScale: 1, linkWidthScale: 1, - arrowSizeScale: 1, + // Deprecated alias (remove in next major) + arrowSizeScale: 1, + // Primary default for `config.linkArrowsSizeScale` + linkArrowsSizeScale: 1, renderLinks: true, curvedLinks: false,• src/config.ts (around line 602)
- public linkArrowsSizeScale = defaultConfigValues.arrowSizeScale + public linkArrowsSizeScale = defaultConfigValues.linkArrowsSizeScaleWith these changes:
defaultConfigValues.linkArrowsSizeScalealigns with the property named in the Config interface and docs.- The deprecated
arrowSizeScalekey will continue to feed the existing assignment until removal in a future major release.- All references in src/modules/Lines/index.ts (and the GLSL uniform
arrowSizeScale) remain correct.
🧹 Nitpick comments (5)
src/stories/3. api-reference.mdx (2)
11-18: Clarify dontRescale semantics and tighten wordingThe param docs are correct, but a tiny wording tweak improves clarity and avoids contractions; also add a short note about the simulation-disabled default to match Points.updatePositions behavior.
Apply:
-* **`dontRescale`** (Boolean, optional): For this call only, controls whether to rescale the points. - - `true`: Don't rescale the points. - - `false` or `undefined` (default): Use the behavior defined by `config.rescalePositions`. +* **`dontRescale`** (Boolean, optional): For this call only, controls whether to rescale the points. + - `true`: Do not rescale the points. + - `false` or `undefined` (default): Use the behavior defined by `config.rescalePositions`. + - Note: When `config.rescalePositions` is `undefined` and simulation is disabled, positions are rescaled by default.Consider adding a second example line showing the flag in use:
graph.setPointPositions(new Float32Array([1, 2, 3, 4, 5, 6])); +// Skip rescaling just for this update: +graph.setPointPositions(new Float32Array([10, 20, 30, 40, 50, 60]), true); graph.render();
252-265: Soften “0–1” constraint; align with implementation or state as recommendationDocs say strengths “typically range from 0.0 to 1.0”. Unless the force actually clamps/normalizes, this reads as a hard constraint. Suggest phrasing it as “recommended” and describing the effect qualitatively. Also call out that array length must match link count (you already do—good).
Proposed change:
-* **`linkStrength`** (Float32Array): A Float32Array representing the strength of each link in the format `[strength1, strength2, ..., strengthN]`, where each `strength` is a numerical value. The strength values typically range from 0.0 to 1.0, with higher values indicating stronger links. The length of the `linkStrength` array should match the number of links in the graph. +* **`linkStrength`** (Float32Array): A Float32Array of per-link coefficients in the format `[strength1, strength2, ..., strengthN]`. Higher values increase the link’s effect in the simulation. Recommended range: `0.0–1.0`. The length of the array must match the number of links.If the engine clamps values, we should document that. Otherwise, this wording avoids over-promising.
src/variables.ts (1)
12-64: Type the default config object to catch future driftTyping
defaultConfigValuesagainst the public config interface prevents silent typos like the arrow size scale mismatch.Example:
-export const defaultConfigValues = { +import type { GraphConfigInterface } from './config' // adjust the path/name if different +export const defaultConfigValues: GraphConfigInterface = { ... }If the interface has required/initialization-only fields, use a Partial or pick the runtime-applicable subset:
export const defaultConfigValues: Partial<GraphConfigInterface> = { ... }src/stories/2. configuration.mdx (2)
30-31: Clarify unit and layering (optional) for hovered link stylingMinor doc enhancement: call out that width increase is in screen pixels and composes with the base per-link width. If zoom scaling also applies, specify ordering (additive before/after scaling). If not, current text is OK.
Suggested tweak:
-| hoveredLinkWidthIncrease | Number of pixels to add to the link width when hovered | `2` | +| hoveredLinkWidthIncrease | Number of screen pixels added to the base link width on hover (composes with per-link widths; specify whether applied before/after zoom scaling) | `2` |
97-99: Document callback signatures + add an examplePlease specify the payload shape (e.g.,
{ index, sourceIndex, targetIndex, screen, space }) and provide a short usage example to remove ambiguity for integrators.Example block to add below the table:
+### Link hover callbacks example + +```ts +graph.setConfig({ + onLinkMouseOver: (evt) => { + // evt: { index: number, sourceIndex: number, targetIndex: number, screen: [number, number], space: [number, number] } + console.log('over link', evt.index) + }, + onLinkMouseOut: (evt) => { + console.log('out link', evt.index) + }, +}) +```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/config.ts(6 hunks)src/modules/Lines/draw-curve-line.vert(5 hunks)src/stories/2. configuration.mdx(3 hunks)src/stories/3. api-reference.mdx(3 hunks)src/variables.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/modules/Lines/draw-curve-line.vert
- src/config.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/variables.ts (3)
src/modules/GraphData/index.ts (1)
updateArrows(270-282)src/modules/Lines/index.ts (8)
updateArrow(151-155)config(67-67)config(71-71)config(78-78)config(76-76)config(74-74)config(66-66)config(70-70)src/index.ts (1)
setLinkArrows(496-500)
src/stories/3. api-reference.mdx (2)
src/index.ts (3)
setPointPositions(300-316)render(571-597)update(1224-1232)src/modules/Points/index.ts (2)
updatePositions(73-178)rescaleInitialNodePositions(880-933)
🪛 LanguageTool
src/stories/2. configuration.mdx
[grammar] ~19-~19: There might be a mistake here.
Context: ...use when hovering over a link | auto | | renderHoveredPointRing | Turns ring re...
(QB_NEW_EN)
src/stories/3. api-reference.mdx
[grammar] ~16-~16: There might be a mistake here.
Context: ... controls whether to rescale the points. - true: Don't rescale the points. - false ...
(QB_NEW_EN)
🔇 Additional comments (5)
src/stories/3. api-reference.mdx (1)
300-305: Render parameter rename and behavior read cleanThe rename to
simulationAlphaand the “zero stops the simulation” note are clear and consistent with the public API. LGTM.src/variables.ts (3)
23-24: Rename consistency: linkArrows default looks goodDefaulting to
linkArrows: falsealigns with GraphData.updateArrows and public APIsetLinkArrows. LGTM.
27-27: Hovered link cursor default added—OKAdding
hoveredLinkCursor: 'auto'keeps parity withhoveredPointCursor. Consider documenting recommended values (e.g., 'pointer') in config docs (which you did)—all good.
30-31: Hovered link styling defaults are sensible
hoveredLinkColorandhoveredLinkWidthIncreasedefaults match the new docs and are practical. No issues.src/stories/2. configuration.mdx (1)
19-19: New hoveredLinkCursor row—LGTMThe description mirrors
hoveredPointCursorand default is consistent with variables. Looks good.
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
src/stories/2. configuration.mdx (1)
94-100: Docs/type mismatch for link mouse-out callbackThe table lists onLinkMouseOver/onLinkMouseOut but doesn’t specify parameters. In src/config.ts, onLinkMouseOut is documented to receive linkIndex, yet the type currently has no params. Align both docs and types; either document the index or remove it from both places. I recommend keeping
(linkIndex: number) => voidfor symmetry with mouse-over.src/config.ts (1)
420-434: Fix onLinkMouseOut to accept the link index (docs vs. type)Docs say the link index is passed, but the type has no params. Change the type to accept the index.
Apply this diff:
/** * Callback function that will be called when the mouse moves out of a link. * The link index will be passed as the first argument: * `(linkIndex: number) => void`. * Default value: `undefined` */ - onLinkMouseOut?: () => void; + onLinkMouseOut?: (linkIndex: number) => void;src/index.ts (3)
1442-1452: Clear stale link hover when there are no linksIf linksNumber is 0, hoveredLinkIndex can remain set. Clear it proactively here.
Apply this diff:
private findHoveredItem (): void { if (this._isDestroyed || !this._isMouseOnCanvas || !this.reglInstance) return + // Clear stale link hover when there are no links + if (!this.graph.linksNumber && this.store.hoveredLinkIndex !== undefined) { + this.config.onLinkMouseOut?.(this.store.hoveredLinkIndex) + this.store.hoveredLinkIndex = undefined + } if (this._findHoveredItemExecutionCount < MAX_HOVER_DETECTION_DELAY) { this._findHoveredItemExecutionCount += 1 return } this._findHoveredItemExecutionCount = 0 this.findHoveredPoint() if (this.graph.linksNumber) this.findHoveredLine() this.updateCanvasCursor() }Additionally, consider clearing hover on canvas leave to prevent stale state:
// in constructor, around the existing mouseleave handler .on('mouseleave.cosmos', () => { this._isMouseOnCanvas = false if (this.store.hoveredPoint) this.config.onPointMouseOut?.(this.currentEvent) if (this.store.hoveredLinkIndex !== undefined) this.config.onLinkMouseOut?.(this.store.hoveredLinkIndex) this.store.hoveredPoint = undefined this.store.hoveredLinkIndex = undefined this.updateCanvasCursor() })
267-269: Don’t pass undefined into Store.setHoveredLinkColor; add reset handlingPassing undefined will either type-error or break runtime. Handle unsetting explicitly and provide a store reset path.
Apply this diff:
- if (prevConfig.hoveredLinkColor !== this.config.hoveredLinkColor) { - this.store.setHoveredLinkColor(this.config.hoveredLinkColor) - } + if (prevConfig.hoveredLinkColor !== this.config.hoveredLinkColor) { + if (this.config.hoveredLinkColor != null) { + this.store.setHoveredLinkColor(this.config.hoveredLinkColor) + } else { + // Either add this method or allow undefined in setHoveredLinkColor and treat it as reset. + this.store.resetHoveredLinkColor?.() + } + }You’ll also need to update Store:
- Either add resetHoveredLinkColor() that restores the default, or
- Change setHoveredLinkColor to accept
string | [number, number, number, number] | undefinedand treat undefined as “reset.”
1485-1513: Bug: mouseout check fails for link index 0; also pass link index to onLinkMouseOut
- Falsy check misses index 0, so mouseout doesn’t fire.
- If you update the config type, pass the last hovered index to onLinkMouseOut.
Apply this diff:
private findHoveredLine (): void { if (this._isDestroyed || !this.reglInstance || !this.lines) return if (this.store.hoveredPoint) { - if (this.store.hoveredLinkIndex !== undefined) { - this.store.hoveredLinkIndex = undefined - this.config.onLinkMouseOut?.() - } + if (this.store.hoveredLinkIndex !== undefined) { + const prev = this.store.hoveredLinkIndex + this.store.hoveredLinkIndex = undefined + this.config.onLinkMouseOut?.(prev) + } return } this.lines.findHoveredLine() let isMouseover = false let isMouseout = false + let prevHoveredLinkIndex: number | undefined const pixels = readPixels(this.reglInstance, this.lines.hoveredLineIndexFbo as regl.Framebuffer2D) const hoveredLineIndex = pixels[0] as number if (hoveredLineIndex >= 0) { if (this.store.hoveredLinkIndex !== hoveredLineIndex) isMouseover = true this.store.hoveredLinkIndex = hoveredLineIndex } else { - if (this.store.hoveredLinkIndex) isMouseout = true - this.store.hoveredLinkIndex = undefined + if (this.store.hoveredLinkIndex !== undefined) { + isMouseout = true + prevHoveredLinkIndex = this.store.hoveredLinkIndex + this.store.hoveredLinkIndex = undefined + } } if (isMouseover && this.store.hoveredLinkIndex !== undefined) { this.config.onLinkMouseOver?.(this.store.hoveredLinkIndex) } - if (isMouseout) this.config.onLinkMouseOut?.() + if (isMouseout && prevHoveredLinkIndex !== undefined) this.config.onLinkMouseOut?.(prevHoveredLinkIndex) }
🧹 Nitpick comments (4)
src/stories/2. configuration.mdx (2)
19-19: Nit: finalize the sentence and clarify accepted valuesConsider ending the description with a period and explicitly stating that standard CSS cursor values are accepted.
30-31: Clarify units and valid ranges
- Mention “pixels” explicitly for hoveredLinkWidthIncrease and that values should be ≥ 0.
- Keep RGBA value bounds (0–255) consistent with other rows.
src/index.ts (2)
53-56: Avoid hard-coding “2” in docs; reference the throttle constantThe comment says “reaches 2”, but behavior depends on MAX_HOVER_DETECTION_DELAY. Update the wording.
Apply this diff:
- * When the counter reaches 2 (or more), it is reset to 0 and the `findHoveredPoint` or `findHoveredLine` method is executed. + * When the counter reaches the MAX_HOVER_DETECTION_DELAY threshold, it is reset to 0 and `findHoveredPoint`/`findHoveredLine` are executed.
191-193: Constructor: hoveredLinkColor init only when truthy — OK, but ensure reset path existsThis is fine for initial override. Ensure your update path supports resetting to default when config sets hoveredLinkColor to undefined (see setConfig below).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/config.ts(7 hunks)src/index.ts(11 hunks)src/stories/2. configuration.mdx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/config.ts (3)
src/variables.ts (1)
defaultConfigValues(12-64)src/modules/GraphData/index.ts (3)
GraphData(16-383)updateLinkColor(217-243)updateArrows(270-282)src/modules/Lines/index.ts (3)
config(71-71)config(74-74)config(76-76)
src/index.ts (4)
src/modules/Drag/index.ts (1)
Drag(5-33)src/modules/Store/index.ts (1)
Store(13-142)src/modules/Points/index.ts (1)
findHoveredPoint(723-726)src/modules/GraphData/index.ts (1)
GraphData(16-383)
🪛 LanguageTool
src/stories/2. configuration.mdx
[grammar] ~19-~19: There might be a mistake here.
Context: ...use when hovering over a link | auto | | renderHoveredPointRing | Turns ring re...
(QB_NEW_EN)
🔇 Additional comments (11)
src/config.ts (5)
101-106: LGTM: hoveredLinkCursor option addedDescription and default align with defaults in variables.ts.
167-179: LGTM: hoveredLinkColor/hoveredLinkWidthIncreaseType and defaults match variables.ts and MDX docs.
361-373: LGTM: onPointClick signatureSignature is clear and consistent with onClick semantics.
374-384: LGTM: onLinkClick signatureLink index + mouse event is the right pair.
608-626: LGTM: new hovered-link fields and callbacks wired into GraphConfigDefaults sourced from defaultConfigValues; arrowLinks→linkArrows init looks correct.
Also applies to: 653-659
src/index.ts (6)
20-20: LGTM: importing MAX_HOVER_DETECTION_DELAYKeeps hover checks throttled in one place.
1254-1254: LGTM: destruction guard around frame loopPrevents re-entrancy after destroy.
Also applies to: 1330-1333
1262-1264: LGTM: skip hover detection while draggingAvoids waste and jitter during drags.
1392-1392: LGTM: resize guards and link index FBO refreshEarly return on destroyed and updating the link index FBO on resize are correct.
Also applies to: 1410-1411
1516-1523: LGTM: cursor update covers link-hover stateCorrectly applies hoveredLinkCursor when a link is hovered.
1353-1364: LGTM: onPointClick/onLinkClick dispatchPriority order and payloads look correct.
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/modules/Lines/index.ts (1)
251-263: Avoid clearing the default framebuffer if linkIndexFbo is unavailableIf updateLinkIndexFbo fails, clear({framebuffer: undefined}) will wipe the main canvas. Add a safety check and early return with a warn.
public findHoveredLine (): void { if (!this.data.linksNumber || !this.store.isLinkHoveringEnabled) return - if (!this.linkIndexFbo) this.updateLinkIndexFbo() + if (!this.linkIndexFbo) this.updateLinkIndexFbo() + if (!this.linkIndexFbo) { + console.warn('Link hover: linkIndexFbo unavailable; skipping hover pass') + return + } this.reglInstance.clear({ - framebuffer: this.linkIndexFbo as regl.Framebuffer2D, + framebuffer: this.linkIndexFbo as regl.Framebuffer2D, color: [0, 0, 0, 0], })src/index.ts (3)
1453-1463: Clear stale link hover when links disappearIf linksNumber becomes 0, hoveredLinkIndex may linger. Clear it proactively.
private findHoveredItem (): void { if (this._isDestroyed || !this._isMouseOnCanvas || !this.reglInstance) return + if (!this.graph.linksNumber && this.store.hoveredLinkIndex !== undefined) { + this.config.onLinkMouseOut?.() + this.store.hoveredLinkIndex = undefined + } if (this._findHoveredItemExecutionCount < MAX_HOVER_DETECTION_DELAY) {
191-196: Don’t pass undefined to setHoveredLinkColor; support reset-to-defaultCalling setHoveredLinkColor with undefined may fail type checks and prevents reverting to defaults. Guard the call; add a reset path.
- if (this.config.hoveredLinkColor) { + if (this.config.hoveredLinkColor != null) { this.store.setHoveredLinkColor(this.config.hoveredLinkColor) } @@ - if (prevConfig.hoveredLinkColor !== this.config.hoveredLinkColor) { - this.store.setHoveredLinkColor(this.config.hoveredLinkColor) - } + if (prevConfig.hoveredLinkColor !== this.config.hoveredLinkColor) { + if (this.config.hoveredLinkColor != null) { + this.store.setHoveredLinkColor(this.config.hoveredLinkColor) + } else { + this.store.resetHoveredLinkColor?.() + } + }If resetHoveredLinkColor doesn’t exist yet, please add it in Store (or make setHoveredLinkColor accept undefined and treat it as reset).
Also applies to: 269-271
1496-1524: Bug: mouseout check fails for link index 0; use explicit undefined checksFalsy check skips index 0. Switch to explicit undefined comparisons to fire mouseout correctly.
const hoveredLineIndex = pixels[0] as number @@ - if (hoveredLineIndex >= 0) { + if (hoveredLineIndex >= 0) { if (this.store.hoveredLinkIndex !== hoveredLineIndex) isMouseover = true this.store.hoveredLinkIndex = hoveredLineIndex } else { - if (this.store.hoveredLinkIndex) isMouseout = true + if (this.store.hoveredLinkIndex !== undefined) isMouseout = true this.store.hoveredLinkIndex = undefined }
🧹 Nitpick comments (4)
src/modules/Lines/index.ts (2)
141-159: Guard against missing linkIndexFbo in second passIf FBO creation fails, passing undefined as linkIndexTexture can crash the draw. Add a fast return.
if (!this.hoveredLineIndexCommand) { this.hoveredLineIndexCommand = reglInstance({ @@ - uniforms: { - linkIndexTexture: () => this.linkIndexFbo, + uniforms: { + linkIndexTexture: () => this.linkIndexFbo, mousePosition: () => store.screenMousePosition, screenSize: () => store.screenSize, },And in findHoveredLine:
- // Execute the command to read the link index at mouse position - this.hoveredLineIndexCommand?.() + // Execute only if we have a valid index texture + if (this.linkIndexFbo) this.hoveredLineIndexCommand?.()
174-176: Use non-blended state for index pass to avoid value mixingIndex render should not blend; current command always has blending enabled. Make blend.enable dynamic, or create a dedicated index draw command with blending disabled.
- blend: { - enable: true, + blend: { + enable: reglInstance.prop<{ blendEnabled: boolean }, 'blendEnabled'>('blendEnabled'), func: { dstRGB: 'one minus src alpha', srcRGB: 'src alpha', dstAlpha: 'one minus src alpha', srcAlpha: 'one', }, @@ - this.drawCurveCommand?.({ framebuffer: null, renderMode: 0.0 }) + this.drawCurveCommand?.({ framebuffer: null, renderMode: 0.0, blendEnabled: true })And in findHoveredLine:
- this.drawCurveCommand?.({ framebuffer: this.linkIndexFbo, renderMode: 1.0 }) + this.drawCurveCommand?.({ framebuffer: this.linkIndexFbo, renderMode: 1.0, blendEnabled: false })src/index.ts (2)
53-56: Comment drifts from actual throttle constantDoc says “reaches 2,” but you use MAX_HOVER_DETECTION_DELAY (currently 4). Make the comment generic or reference the constant.
- * When the counter reaches 2 (or more), it is reset to 0 and the `findHoveredPoint` or `findHoveredLine` method is executed. + * When the counter reaches MAX_HOVER_DETECTION_DELAY, it resets to 0 and runs hover detection.
124-127: Clear hover state on canvas leave (optional but improves UX)On mouseleave, also clear hoveredPoint/hoveredLinkIndex and fire mouseout callbacks to avoid stale cursor/state.
- .on('mouseleave.cosmos', () => { this._isMouseOnCanvas = false }) + .on('mouseleave.cosmos', (event) => { + this._isMouseOnCanvas = false + if (this.store.hoveredPoint) this.config.onPointMouseOut?.(event) + if (this.store.hoveredLinkIndex !== undefined) this.config.onLinkMouseOut?.() + this.store.hoveredPoint = undefined + this.store.hoveredLinkIndex = undefined + this.updateCanvasCursor() + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/index.ts(12 hunks)src/modules/Lines/index.ts(7 hunks)src/modules/Store/index.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/modules/Store/index.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-31T06:19:00.654Z
Learnt from: Stukova
PR: cosmosgl/graph#169
File: src/modules/Points/index.ts:555-560
Timestamp: 2025-07-31T06:19:00.654Z
Learning: Buffer creation in regl (WebGL) contexts using `reglInstance.buffer()` is treated as a reliable operation in this codebase. The established pattern across all modules is to create buffers without null checks after creation, trusting that if buffer creation fails, it indicates more fundamental WebGL context issues that should bubble up rather than be handled silently.
Applied to files:
src/modules/Lines/index.ts
🧬 Code graph analysis (2)
src/modules/Lines/index.ts (1)
src/modules/core-module.ts (1)
CoreModule(7-28)
src/index.ts (2)
src/modules/Store/index.ts (2)
ALPHA_MIN(8-8)MAX_HOVER_DETECTION_DELAY(16-16)src/helper.ts (1)
readPixels(38-45)
🔇 Additional comments (7)
src/modules/Lines/index.ts (4)
83-112: New linkIndices attribute/uniforms — LGTMAttribute layout and per-instance stride look correct. Uniforms align with the described shader changes. No blockers.
161-164: Fullscreen quad buffer — LGTMPositions cover NDC corners and match triangle strip with count=4.
178-195: FBO resize/update — LGTMResize logic matches screenSize and respects the feature toggle. Good reuse via reconfiguring the same FBO.
218-223: Sequential link indices buffer — LGTMStraightforward and efficient. Matches instance divisor=1.
src/index.ts (3)
1262-1272: Hover check skipped while dragging — LGTMPrevents stalls from readbacks during drag.
1399-1422: Link index FBO refresh on resize — LGTMGated by the feature flag; good.
1361-1372: Link click dispatch — LGTMClick routing between points and links looks correct and preserves existing onClick signature.
Changes made with help of CodeRabbit suggestions Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/config.ts (1)
420-434: Doc signature for onLinkMouseOut now matches implementationThis resolves the earlier mismatch flagged in past reviews.
src/modules/Lines/index.ts (1)
260-272: Guard when FBO creation fails to avoid null framebuffer useIf updateLinkIndexFbo() fails, subsequent clear/draw will use undefined FBO.
- if (!this.linkIndexFbo) this.updateLinkIndexFbo() + if (!this.linkIndexFbo) this.updateLinkIndexFbo() + if (!this.linkIndexFbo) { + console.warn('Lines: linkIndexFbo is not available; skipping link hover detection') + return + }src/index.ts (2)
1455-1465: Proactively clear link hover when links are absent/disabledIf linksNumber is 0/undefined or link hovering disabled, hoveredLinkIndex can linger.
private findHoveredItem (): void { if (this._isDestroyed || !this._isMouseOnCanvas || !this.reglInstance) return + if ((!this.graph.linksNumber || !this.store.isLinkHoveringEnabled) && this.store.hoveredLinkIndex !== undefined) { + this.config.onLinkMouseOut?.() + this.store.hoveredLinkIndex = undefined + } if (this._findHoveredItemExecutionCount < MAX_HOVER_DETECTION_DELAY) {
269-271: Guard against undefinedhoveredLinkColorbefore calling setterThe
Store.setHoveredLinkColorsignature only acceptsstring | [number,number,number,number], so passing inundefined(whenconfig.hoveredLinkColoris cleared) will cause a runtime error and won’t reset the color back to its default.Locations to update:
src/index.tsat lines 269–271- Add a corresponding reset in
src/modules/Store/index.tsSuggested changes:
--- a/src/index.ts +++ b/src/index.ts @@ -268,7 +268,14 @@ if (prevConfig.hoveredLinkColor !== this.config.hoveredLinkColor) { - this.store.setHoveredLinkColor(this.config.hoveredLinkColor) + if (this.config.hoveredLinkColor != null) { + this.store.setHoveredLinkColor(this.config.hoveredLinkColor) + } else { + // reset to default + // introduce resetHoveredLinkColor() in Store + this.store.resetHoveredLinkColor() + } }--- a/src/modules/Store/index.ts +++ b/src/modules/Store/index.ts @@ -147,6 +147,12 @@ public setHoveredLinkColor(color: string | [number, number, number, number]): void { const convertedRgba = getRgbaColor(color) this.hoveredLinkColor[0] = convertedRgba[0] + this.hoveredLinkColor[1] = convertedRgba[1] + this.hoveredLinkColor[2] = convertedRgba[2] + this.hoveredLinkColor[3] = convertedRgba[3] } + + /** Restore to default RGBA value */ + public resetHoveredLinkColor(): void { + this.hoveredLinkColor = [...this.defaultHoveredLinkColor] + }If you’d rather not add a reset method, you can instead widen
setHoveredLinkColorto acceptundefinedand internally reset whencoloris nullish.
🧹 Nitpick comments (4)
src/modules/Lines/draw-curve-line.vert (1)
81-84: Avoid exact float equality for hover checksComparing hoveredLinkIndex and linkIndex with == risks precision issues for large indices. Use an epsilon compare.
Apply:
- if (hoveredLinkIndex == linkIndex) { + if (abs(hoveredLinkIndex - linkIndex) < 0.5) { linkWidthPx += hoveredLinkWidthIncrease / transformationMatrix[0][0]; }- if (hoveredLinkIndex == linkIndex) { + if (abs(hoveredLinkIndex - linkIndex) < 0.5) { rgbaColor.rgb = hoveredLinkColor.rgb; rgbaColor.a *= hoveredLinkColor.a; }Also applies to: 136-144, 164-169
src/modules/Lines/index.ts (2)
29-41: Gate hoveredLineIndexFbo creation behind feature flagYou already gate linkIndexFbo; do the same for hoveredLineIndexFbo to avoid unnecessary allocations when link hovering is disabled.
- // Initialize the hovered line index FBO - if (!this.hoveredLineIndexFbo) { + // Initialize the hovered line index FBO only when link hovering is enabled + if (this.store.isLinkHoveringEnabled && !this.hoveredLineIndexFbo) { this.hoveredLineIndexFbo = reglInstance.framebuffer({ color: reglInstance.texture({ width: 1, height: 1, format: 'rgba', type: 'float', }), depth: false, stencil: false, }) }
117-139: Make index-pass blending explicit to avoid surprisesCurrent comment assumes alpha=1.0 to “overwrite.” Safer is to disable blending (or use ONE, ZERO) for renderMode=1.0. With a single command, you can duplicate the draw command for the index pass with blending disabled.
If you split commands:
// create a second draw command for index rendering: + this.drawCurveIndexCommand = reglInstance({ + vert: drawLineVert, + frag: drawLineFrag, + // ...same attributes/uniforms but hardcode renderMode: 1.0 + uniforms: { ... , renderMode: 1.0 }, + blend: { enable: false }, + depth: { enable: false, mask: false }, + framebuffer: reglInstance.prop('framebuffer'), + // ... + })Then in findHoveredLine:
- this.drawCurveCommand?.({ framebuffer: this.linkIndexFbo, renderMode: 1.0 }) + this.drawCurveIndexCommand?.({ framebuffer: this.linkIndexFbo })src/index.ts (1)
124-127: Clear hover state on mouseleave to avoid stale cursor/stateCurrently only flips _isMouseOnCanvas. Clear hovered point/link and fire respective mouseout callbacks.
- .on('mouseleave.cosmos', () => { this._isMouseOnCanvas = false }) + .on('mouseleave.cosmos', () => { + this._isMouseOnCanvas = false + if (this.store.hoveredPoint) { + this.config.onPointMouseOut?.(this.currentEvent) + this.store.hoveredPoint = undefined + } + if (this.store.hoveredLinkIndex !== undefined) { + this.config.onLinkMouseOut?.() + this.store.hoveredLinkIndex = undefined + } + this.updateCanvasCursor() + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/config.ts(7 hunks)src/index.ts(14 hunks)src/modules/Lines/draw-curve-line.vert(6 hunks)src/modules/Lines/index.ts(7 hunks)src/variables.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-27T11:17:17.046Z
Learnt from: Stukova
PR: cosmosgl/graph#170
File: src/modules/Lines/index.ts:29-41
Timestamp: 2025-08-27T11:17:17.046Z
Learning: OES_texture_float extension implicitly enables WEBGL_color_buffer_float extension in WebGL 1 contexts, making it unnecessary to explicitly request WEBGL_color_buffer_float when using float render targets. WEBGL_color_buffer_float is for WebGL 1, while EXT_color_buffer_float is for WebGL 2.
Applied to files:
src/modules/Lines/index.ts
📚 Learning: 2025-07-31T06:19:00.654Z
Learnt from: Stukova
PR: cosmosgl/graph#169
File: src/modules/Points/index.ts:555-560
Timestamp: 2025-07-31T06:19:00.654Z
Learning: Buffer creation in regl (WebGL) contexts using `reglInstance.buffer()` is treated as a reliable operation in this codebase. The established pattern across all modules is to create buffers without null checks after creation, trusting that if buffer creation fails, it indicates more fundamental WebGL context issues that should bubble up rather than be handled silently.
Applied to files:
src/modules/Lines/index.ts
🧬 Code graph analysis (5)
src/config.ts (2)
src/variables.ts (1)
defaultConfigValues(12-64)src/modules/GraphData/index.ts (3)
GraphData(16-383)updateArrows(270-282)updateLinkColor(217-243)
src/variables.ts (1)
src/modules/GraphData/index.ts (1)
updateArrows(270-282)
src/index.ts (6)
src/modules/Store/index.ts (3)
ALPHA_MIN(8-8)MAX_HOVER_DETECTION_DELAY(16-16)Store(13-142)src/helper.ts (1)
readPixels(38-45)src/modules/Drag/index.ts (1)
Drag(5-33)src/modules/Zoom/index.ts (1)
Zoom(8-148)src/modules/GraphData/index.ts (1)
GraphData(16-383)src/modules/Points/index.ts (1)
findHoveredPoint(723-726)
src/modules/Lines/draw-curve-line.vert (1)
src/modules/GraphData/index.ts (1)
updateArrows(270-282)
src/modules/Lines/index.ts (2)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Points/index.ts (1)
Points(25-934)
🔇 Additional comments (16)
src/modules/Lines/draw-curve-line.vert (2)
14-15: Uniform rename alignment looks goodlinkArrowsSizeScale is consistently applied to arrowWidth. No issues spotted.
Also applies to: 113-114
136-144: Good: extra width for picking modeAdding 5 px padding in index-render mode improves hover stability without affecting visual render.
src/variables.ts (1)
17-17: Defaults added/renamed correctly
- linkArrowsSizeScale and linkArrows rename align with usage.
- hoveredLinkCursor, hoveredLinkColor, hoveredLinkWidthIncrease defaults look sane.
Also applies to: 23-31
src/config.ts (4)
101-106: New hoveredLinkCursor option documented and typedConsistent with point cursor setting.
166-179: hoveredLinkColor/WidthIncrease API additions are coherentTypes and docs match existing color conventions.
361-383: Point/link click callbacks: API surface looks cleanSignatures are explicit and align with usage in src/index.ts.
607-626: Config->class field mapping is completeNew fields are wired into GraphConfig with defaults from variables.ts.
Also applies to: 651-659
src/modules/Lines/index.ts (3)
83-89: Attribute/uniform plumbing LGTMlinkIndices, hoveredLinkIndex/color/widthIncrease, and renderMode are wired correctly into the draw command.
Also applies to: 108-112
187-204: FBO sizing approach is consistent with shader usageUsing store.screenSize for the index texture matches your shader’s screen-space math. No change requested.
170-174: Quad buffer init for fullscreen pass — LGTMSimple triangle strip with 4 verts is fine here.
src/index.ts (6)
191-196: Initialize link-hover feature flag and color — LGTMPropagates hoveredLinkColor and enables link-hover subsystem at construction.
1272-1274: Nice: skip hover detection during dragPrevents expensive hover work while dragging.
1420-1423: Resize hook updates link index FBO only when enabledGood guard; matches allocation strategy.
1498-1526: Hover line detection flow is sound
- Clears link hover when a point is hovered.
- Emits mouseover/mouseout appropriately.
- Uses >= 0 sentinel for valid index.
1529-1536: Cursor update handles index 0 correctlyExplicit undefined check avoids falsy bug for link index 0.
295-301: No missing onLinkClick check –updateLinkHoveringEnabledalready handles itThe
Store.updateLinkHoveringEnabledmethod’s signature and implementation explicitly includeonLinkClickin the enabling logic:
- Signature picks up onLinkClick, onLinkMouseOver, onLinkMouseOut from the config
- Implementation sets
isLinkHoveringEnabled = !!(config.onLinkClick || config.onLinkMouseOver || config.onLinkMouseOut)(src/modules/Store/index.ts:143–145)Because
onLinkClickis already OR’ed in alongside the mouse-over and mouse-out callbacks, no additional change is needed to cover link clicks in the hover-enablement step.Likely an incorrect or invalid review comment.
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/index.ts (3)
263-265: Allow unsetting hoveredLinkColor (propagate undefined as reset).Current logic coalesces to a hard default, so users can’t “unset” an override later.
- if (prevConfig.hoveredLinkColor !== this.config.hoveredLinkColor) { - this.store.setHoveredLinkColor(this.config.hoveredLinkColor ?? defaultConfigValues.hoveredLinkColor) - } + if (prevConfig.hoveredLinkColor !== this.config.hoveredLinkColor) { + if (this.config.hoveredLinkColor != null) { + this.store.setHoveredLinkColor(this.config.hoveredLinkColor) + } else { + // either add this method or make setHoveredLinkColor accept undefined as “reset” + this.store.resetHoveredLinkColor?.() + } + }If you prefer not to add a reset method, update Store.setHoveredLinkColor to accept undefined and treat it as reset internally (types + usages).
123-127: Also clear hover on canvas leave to avoid stale state.Currently only flips the canvas presence flag; stale hover and cursor can persist.
- .on('mouseleave.cosmos', () => { this._isMouseOnCanvas = false }) + .on('mouseleave.cosmos', () => { + this._isMouseOnCanvas = false + if (this.store.hoveredPoint) this.config.onPointMouseOut?.(this.currentEvent) + if (this.store.hoveredLinkIndex !== undefined) this.config.onLinkMouseOut?.() + this.store.hoveredPoint = undefined + this.store.hoveredLinkIndex = undefined + this.updateCanvasCursor() + })
1449-1460: Clear stale link hover when linksNumber is 0.If links disappear, findHoveredLine isn’t called and hoveredLinkIndex can stick, leaving stale cursor/state.
private findHoveredItem (): void { if (this._isDestroyed || !this._isMouseOnCanvas || !this.reglInstance) return + // Clear stale link hover when there are no links + if (!this.graph.linksNumber && this.store.hoveredLinkIndex !== undefined) { + this.config.onLinkMouseOut?.() + this.store.hoveredLinkIndex = undefined + } if (this._findHoveredItemExecutionCount < MAX_HOVER_DETECTION_DELAY) { this._findHoveredItemExecutionCount += 1 return }
🧹 Nitpick comments (4)
src/index.ts (4)
53-56: Docstring out of date with the constant.Comment says “reaches 2 (or more)” but the code uses MAX_HOVER_DETECTION_DELAY (currently 4). Suggest referencing the constant to avoid drift.
- * When the counter reaches 2 (or more), it is reset to 0 and the `findHoveredPoint` or `findHoveredLine` method is executed. + * When the counter reaches MAX_HOVER_DETECTION_DELAY, it is reset to 0 and `findHoveredPoint`/`findHoveredLine` are executed.Also applies to: 58-59
291-295: Recomputing link-hover enablement on callback changes — LGTM; consider FBO refresh.Looks good. Optionally refresh the index FBO when enabling hover to avoid waiting for a resize.
if (prevConfig.onLinkClick !== this.config.onLinkClick || prevConfig.onLinkMouseOver !== this.config.onLinkMouseOver || prevConfig.onLinkMouseOut !== this.config.onLinkMouseOut) { this.store.updateLinkHoveringEnabled(this.config) + if (this.store.isLinkHoveringEnabled) this.lines?.updateLinkIndexFbo() }
1461-1490: Use an explicit check for pointSize.Guard on truthiness can misbehave for very small values; compare numerically.
- if (pointSize) { + if (pointSize > 0) {
1238-1242: Optional: clear hoveredLinkIndex on update() re-init.You reset hoveredPoint during re-init; consider also clearing hoveredLinkIndex to avoid stale link hover after recreation.
this.create() this.initPrograms() this.store.hoveredPoint = undefined + this.store.hoveredLinkIndex = undefined this.start(simulationAlpha)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/index.ts(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/index.ts (3)
src/variables.ts (3)
defaultConfigValues(12-64)defaultGreyoutPointColor(3-3)defaultBackgroundColor(10-10)src/helper.ts (2)
getRgbaColor(16-32)readPixels(38-45)src/modules/Store/index.ts (3)
ALPHA_MIN(8-8)MAX_HOVER_DETECTION_DELAY(16-16)Store(13-142)
🔇 Additional comments (10)
src/index.ts (10)
20-20: Imports look correct; new deps are used.Brings MAX_HOVER_DETECTION_DELAY and default colors into scope and they’re referenced below. No issues.
Also applies to: 23-24
178-186: Constructor color propagation — LGTM.Background/point-ring/focused-ring/greyout/hovered-link color setup is consistent with defaults. No issues.
251-253: Background color fallback — LGTM.Using defaultBackgroundColor on undefined is correct and consistent.
255-259: Ring colors fallback — LGTM.Nullish fallback to defaults is appropriate.
261-262: Greyout color reset — LGTM.Passing undefined to setGreyoutPointColor maps to the sentinel [-1,-1,-1,-1] per Store; good.
1258-1258: Frame loop guards and hover scheduling — LGTM.Destroyed-instance guard, skip-while-dragging, and re-entrancy check are sound.
Also applies to: 1266-1268, 1334-1336
1356-1368: Click routing for point vs. link — LGTM.Back-compat on onClick plus specific onPointClick/onLinkClick is clear and correct.
1396-1396: Resize guards — LGTM; FBO refresh conditional is right.Early return on destroyed is safe. Updating link index FBO only when hover is enabled is correct.
Also applies to: 1414-1417
1492-1520: Link hover transitions — LGTM; index 0 handled correctly.Explicit undefined checks fix the prior “index 0” mouseout bug. Good separation from point hover.
1523-1530: Cursor update for link hover — LGTM.Applies hoveredLinkCursor only when enabled and a link is hovered. Correct.
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/index.ts (2)
1466-1487: Simplify stale-link clearing branchMinor cleanup: you already know the branch executes only when link-hover is disabled or there are no links; the
wasHoveredrecheck is redundant.- if (this.graph.linksNumber && this.store.isLinkHoveringEnabled) { + if (this.graph.linksNumber && this.store.isLinkHoveringEnabled) { this.findHoveredLine() - } else if (this.store.hoveredLinkIndex !== undefined) { - // Clear stale hoveredLinkIndex when there are no links - const wasHovered = this.store.hoveredLinkIndex !== undefined - this.store.hoveredLinkIndex = undefined - if (wasHovered && this.config.onLinkMouseOut) { - this.config.onLinkMouseOut() - } - } + } else if (this.store.hoveredLinkIndex !== undefined) { + this.store.hoveredLinkIndex = undefined + this.config.onLinkMouseOut?.() + }
1398-1401: Useevent.buttonsto detect right-button drag reliably
event.whichis deprecated and unreliable on mousemove.buttonsbitmask avoids false negatives during movement.- this.isRightClickMouse = event.which === 3 + // Right button is bit 2 in the buttons bitmask + this.isRightClickMouse = (event.buttons & 2) !== 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/index.ts(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/index.ts (5)
src/variables.ts (3)
defaultConfigValues(12-64)defaultGreyoutPointColor(3-3)defaultBackgroundColor(10-10)src/helper.ts (2)
getRgbaColor(16-32)readPixels(38-45)src/modules/Store/index.ts (3)
ALPHA_MIN(8-8)MAX_HOVER_DETECTION_DELAY(16-16)Store(13-142)src/modules/GraphData/index.ts (1)
GraphData(16-383)src/modules/Points/index.ts (1)
findHoveredPoint(723-726)
🔇 Additional comments (11)
src/index.ts (11)
20-24: LGTM: imports for hover/link defaults and throttle look correct
MAX_HOVER_DETECTION_DELAYand default color imports are consistent with usage. No issues.
53-60: LGTM: unified hover throttle counterRenaming and repurposing the execution counter for item-level hover detection is clear and correct.
195-205: LGTM: initial store visual state propagationBackground, ring colors, greyout, hovered link color, and hover-enable toggle are wired correctly.
268-276: LGTM: live config updates for colors and link-hover togglingFallbacks via defaults are consistent; hover enable flag updates are downstream-safe.
Also applies to: 278-282, 307-313
1275-1275: LGTM: destruction guards in frame loopEarly-return and tail-recursion guard prevent post-destroy re-entrancy.
Also applies to: 1351-1353
1283-1286: LGTM: hover detection paused during dragSkipping hover checks while dragging avoids unnecessary FBO reads.
1374-1385: LGTM: click routing between points and linksOrder and optional chaining prevent spurious calls; link click path is added correctly.
1413-1413: LGTM: resize guard and conditional FBO updateNo work after destroy; link-index FBO updated only when hovering is enabled.
Also applies to: 1432-1434
1489-1519: LGTM: point hover detection and eventsExplicit mouseover/mouseout flags and callback invocations are correct.
1520-1548: LGTM: link hover detection with point precedence
- Clears link hover when a point is hovered.
- Uses
!== undefinedfor index 0 correctness.- Event firing is properly gated.
1551-1558: LGTM: cursor update includes link-hover cursorPriority order (drag > point > link > default) is sensible.
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/config.ts (1)
420-427: Consider parity: pass event to onLinkMouseOver tooFor API symmetry with onLinkMouseOut (which receives the event), consider adding the event parameter to onLinkMouseOver. Optional, since link index alone may suffice.
- onLinkMouseOver?: (linkIndex: number) => void; + onLinkMouseOver?: ( + linkIndex: number, + event?: MouseEvent | D3ZoomEvent<HTMLCanvasElement, undefined> | D3DragEvent<HTMLCanvasElement, undefined, Hovered> + ) => void;src/index.ts (2)
53-57: Doc says “reaches 2” but code uses MAX_HOVER_DETECTION_DELAYAlign the comment with the constant to avoid confusion.
- * When the counter reaches 2 (or more), it is reset to 0 and the `findHoveredPoint` or `findHoveredLine` method is executed. + * When the counter reaches MAX_HOVER_DETECTION_DELAY (or more), it is reset to 0 + * and the `findHoveredPoint` or `findHoveredLine` method is executed.
1472-1493: findHoveredItem: good orchestration; minor cleanupLogic is sound. Micro: wasHovered is redundant in the stale-clear branch.
- } else if (this.store.hoveredLinkIndex !== undefined) { - // Clear stale hoveredLinkIndex when there are no links - const wasHovered = this.store.hoveredLinkIndex !== undefined - this.store.hoveredLinkIndex = undefined - if (wasHovered && this.config.onLinkMouseOut) { - this.config.onLinkMouseOut(this.currentEvent) - } - } + } else if (this.store.hoveredLinkIndex !== undefined) { + // Clear stale hoveredLinkIndex when there are no links + this.config.onLinkMouseOut?.(this.currentEvent) + this.store.hoveredLinkIndex = undefined + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/config.ts(7 hunks)src/index.ts(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/config.ts (4)
src/modules/Store/index.ts (1)
Hovered(18-18)src/variables.ts (1)
defaultConfigValues(12-64)src/modules/GraphData/index.ts (3)
GraphData(16-383)updateLinkColor(217-243)updateArrows(270-282)src/modules/Lines/index.ts (3)
config(74-74)config(71-71)config(76-76)
src/index.ts (6)
src/variables.ts (3)
defaultConfigValues(12-64)defaultGreyoutPointColor(3-3)defaultBackgroundColor(10-10)src/helper.ts (2)
getRgbaColor(16-32)readPixels(38-45)src/modules/Store/index.ts (3)
ALPHA_MIN(8-8)MAX_HOVER_DETECTION_DELAY(16-16)Store(13-142)src/modules/Drag/index.ts (1)
Drag(5-33)src/modules/Points/index.ts (1)
findHoveredPoint(723-726)src/modules/GraphData/index.ts (1)
GraphData(16-383)
🔇 Additional comments (22)
src/config.ts (9)
101-106: Add hoveredLinkCursor — LGTMConsistent with hoveredPointCursor and used correctly in updateCanvasCursor.
167-179: Hovered link styling API — LGTMTypes and defaults match defaultConfigValues; semantics are clear.
361-373: onPointClick signature — LGTMMatches usage in src/index.ts and follows existing onClick docs.
374-384: onLinkClick signature — LGTMMatches usage in src/index.ts; clear two-arg shape.
428-435: onLinkMouseOut docs/signature — LGTMNow aligned with implementation; event is passed through.
608-608: GraphConfig.hoveredLinkCursor default wiring — LGTMCorrectly reads from defaultConfigValues.
618-620: GraphConfig hoveredLinkColor/WidthIncrease — LGTMDefaults correctly applied; names align with variables.ts.
625-626: Renamed arrow settings — LGTMRenames match variables.ts (linkArrows/linkArrowsSizeScale).
653-660: Config fields for new callbacks — LGTMTypes match GraphConfigInterface; initialized to undefined.
src/index.ts (13)
20-24: Updated imports — LGTMBrings in MAX_HOVER_DETECTION_DELAY and default color fallbacks used below.
58-60: Mouse-on-canvas guard — LGTMExplains the throttle guard clearly.
126-149: Mouseleave: clear hover state and pass real event — LGTMGood fixes: pass the actual event to callbacks, reset right-click flag, and update cursor.
201-211: Initial store color/hover setup — LGTMNullish-coalescing to defaults prevents undefined from leaking; enabling link-hovering routed via Store.
274-288: setConfig: background/ring/greyout/hoveredLinkColor updates — LGTMConsistent fallback to defaults; avoids passing undefined into store setters.
1281-1292: Frame guards + hover detection only when not dragging — LGTMPrevents work after destroy and avoids hover churn during drag.
Also applies to: 1357-1359
1380-1391: Click dispatch for point vs link — LGTMClear precedence: point click wins over link click.
1419-1441: Resize: early return + FBO refresh when enabled — LGTMAvoids unnecessary FBO updates and supports dynamic canvas size.
1495-1524: findHoveredPoint — LGTMCorrectly reads FBO, debounces events, and forwards current event.
1526-1554: findHoveredLine — LGTMGood: clears link hover when a point is hovered; emits over/out appropriately; uses explicit undefined checks to handle index 0.
1557-1564: Cursor update for hovered links — LGTMRespects isLinkHoveringEnabled and applies hoveredLinkCursor.
314-318: Link-hover reset logic confirmed and approvedI verified that
updateLinkHoveringEnabledin src/modules/Store/index.ts (lines 143–146) correctly:
- Sets
isLinkHoveringEnabledbased on the presence of any link callbacks- Clears
hoveredLinkIndex(undefined) when disabling link-hoveringNo further changes needed—approving these updates.
420-434: All onLinkMouseOut calls now pass an event; no zero-arg calls remainRan a repository-wide search in
.ts/.tsxfiles and found only two invocation sites—both supplying an argument:
- src/index.ts:137 →
this.config.onLinkMouseOut(event)- src/index.ts:1488 →
this.config.onLinkMouseOut(this.currentEvent)No calls to onLinkMouseOut() without parameters were detected.
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
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 (4)
src/stories/3. api-reference.mdx (3)
119-121: Remove duplicate sentence“Images are rendered above shapes.” appears twice back-to-back.
This method sets the images for the graph points using [ImageData](https://developer.mozilla.org/en-US/docs/Web/API/ImageData) objects. Images are rendered above shapes. To use images, provide image indices via `setPointImageIndices()`. - -Images are rendered above shapes.
430-433: Fix typo: repeated word “method”User-facing docs; correct the duplication.
-This method method is used to convert X and Y point coordinates from the space coordinate system (which is typically used for graph data) to the screen coordinate system (which is used for rendering the graph on the display). +This method converts X and Y point coordinates from the space coordinate system (typically used for graph data) to the screen coordinate system (used for rendering).
436-438: Fix stray backtick and wording-This` method converts X and Y point coordinates from the screen coordinate system (which is used for rendering the graph on the display) to the space coordinate system (which is typically used for graph data). +This method converts X and Y point coordinates from the screen coordinate system (used for rendering) to the space coordinate system (used for graph data).src/index.ts (1)
1415-1418: Use standard MouseEvent.buttons for right-click detectionevent.which is deprecated and unreliable across browsers.
- this.isRightClickMouse = event.which === 3 + // Secondary (right) button bit (2) in the standard buttons mask + this.isRightClickMouse = !!((event.buttons ?? 0) & 2)
🧹 Nitpick comments (3)
src/stories/3. api-reference.mdx (1)
16-18: Nit: tighten phrasing for dontRescale bulletsAvoid contraction; clarify default behavior.
-* **`dontRescale`** (Boolean, optional): For this call only, controls whether to rescale the points. - - `true`: Don't rescale the points. - - `false` or `undefined` (default): Use the behavior defined by `config.rescalePositions`. +* **`dontRescale`** (Boolean, optional): For this call only, controls whether to rescale the points. + - `true`: Do not rescale points. + - `false` or `undefined` (default): Uses the behavior defined by `config.rescalePositions`.src/stories/2. configuration.mdx (1)
84-102: Document when link-hover detection is activeThe implementation enables GPU link-picking only when any of onLinkClick/onLinkMouseOver/onLinkMouseOut is provided. Add a short note to set expectations and avoid confusion when callbacks are omitted.
Example addition (below the callbacks table):
- “Note: Link-hover detection is enabled automatically when at least one of onLinkClick, onLinkMouseOver, or onLinkMouseOut is provided. Disabling all three turns off link-hovering to save GPU work.”
src/index.ts (1)
51-57: Comment drifts from constantThe comment still says “reaches 2”, but you use MAX_HOVER_DETECTION_DELAY (currently 4). Make the comment match the constant.
- * When the counter reaches 2 (or more), it is reset to 0 and the `findHoveredPoint` or `findHoveredLine` method is executed. + * When the counter reaches `MAX_HOVER_DETECTION_DELAY`, it is reset to 0 and `findHoveredPoint`/`findHoveredLine` are executed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/config.ts(8 hunks)src/index.ts(15 hunks)src/stories/2. configuration.mdx(3 hunks)src/stories/3. api-reference.mdx(4 hunks)src/stories/beginners/basic-set-up/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/stories/beginners/basic-set-up/index.ts (2)
src/index.ts (2)
unpause(1112-1116)pause(1102-1106)src/stories/beginners/remove-points/index.ts (3)
pause(74-78)start(80-84)togglePause(86-89)
src/config.ts (4)
src/modules/Store/index.ts (1)
Hovered(18-18)src/variables.ts (1)
defaultConfigValues(12-64)src/modules/GraphData/index.ts (4)
GraphData(16-383)updateLinkColor(217-243)update(319-337)updateLinkWidth(248-265)src/modules/Drag/index.ts (1)
Drag(5-33)
src/index.ts (6)
src/variables.ts (3)
defaultConfigValues(12-64)defaultGreyoutPointColor(3-3)defaultBackgroundColor(10-10)src/helper.ts (2)
getRgbaColor(16-32)readPixels(38-45)src/modules/Store/index.ts (3)
ALPHA_MIN(8-8)MAX_HOVER_DETECTION_DELAY(16-16)Store(13-142)src/modules/Drag/index.ts (1)
Drag(5-33)src/modules/Points/index.ts (1)
findHoveredPoint(723-726)src/modules/GraphData/index.ts (1)
GraphData(16-383)
src/stories/3. api-reference.mdx (2)
src/stories/beginners/remove-points/index.ts (3)
pause(74-78)start(80-84)graph(6-94)src/stories/geospatial/moscow-metro-stations/index.ts (1)
rescalePositions(52-59)
🪛 LanguageTool
src/stories/2. configuration.mdx
[grammar] ~19-~19: There might be a mistake here.
Context: ...use when hovering over a link | auto | | renderHoveredPointRing | Turns ring re...
(QB_NEW_EN)
src/stories/3. api-reference.mdx
[grammar] ~16-~16: There might be a mistake here.
Context: ... controls whether to rescale the points. - true: Don't rescale the points. - false ...
(QB_NEW_EN)
🔇 Additional comments (6)
src/stories/beginners/basic-set-up/index.ts (2)
74-83: Unpause logic — LGTMSelecting start() vs unpause() based on graph.progress is correct and matches the API semantics.
90-95: Avoid double-calling pause on simulation endonSimulationEnd already sets isSimulationRunning=false; calling pause() here will also fire onSimulationPause, which may be unintended. Update the UI without invoking graph.pause().
- graph.setConfig({ - onSimulationEnd: (): void => { - pause() - }, - }) + graph.setConfig({ + onSimulationEnd: (): void => { + isPaused = true + pauseButton.textContent = 'Start' + }, + })src/config.ts (1)
426-441: Link hover callbacks: signatures and docs are consistent — niceonLinkMouseOver(index) and onLinkMouseOut(event) align with usage; deprecation note for onSimulationRestart is clear.
src/index.ts (3)
126-149: Mouseleave cleanup — LGTMPassing the real event to onPointMouseOut/onLinkMouseOut and resetting flags/cursor prevents stale hover state.
1483-1504: Unified hover orchestration — LGTMThrottled findHoveredItem(), conditional line-hover with cleanup when disabled/no links, and cursor refresh are solid.
1550-1565: Link hover state transitions — LGTMExplicit undefined checks avoid the index 0 bug; mouseover/mouseout callbacks fire correctly.
…groundClick Signed-off-by: Stukova Olya <stukova.o@gmail.com>
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/index.ts (1)
1596-1600: Harden target=_blank links (reverse tabnabbing).Auto-add rel="noopener noreferrer" when target="_blank".
- this.attributionDivElement.innerHTML = sanitizeHtml(this.config.attribution, { - ALLOWED_TAGS: ['a', 'b', 'i', 'em', 'strong', 'span', 'div', 'p', 'br', 'img'], - ALLOWED_ATTR: ['href', 'target', 'class', 'id', 'style', 'src', 'alt', 'title'], - }) + this.attributionDivElement.innerHTML = sanitizeHtml(this.config.attribution, { + ALLOWED_TAGS: ['a', 'b', 'i', 'em', 'strong', 'span', 'div', 'p', 'br', 'img'], + ALLOWED_ATTR: ['href', 'target', 'rel', 'class', 'id', 'style', 'src', 'alt', 'title'], + transformTags: { + a: (tagName, attribs) => ({ + tagName, + attribs: attribs.target === '_blank' + ? { ...attribs, rel: attribs.rel ?? 'noopener noreferrer' } + : attribs + }) + } + })
♻️ Duplicate comments (2)
src/index.ts (2)
126-149: Mouseleave handler: correct event plumb-through and state reset — LGTM.
201-209: Verify Store ring color setters accept RGBA arrays.Config allows string | [r,g,b,a], but Store.setHoveredPointRingColor/setFocusedPointRingColor historically took string only. Ensure Store signatures accept both and normalize internally.
#!/bin/bash # Expect both setters to accept string | [number,number,number,number] rg -n -C2 'setHoveredPointRingColor\s*\(' src | sed -n '1,120p' rg -n -C2 'setFocusedPointRingColor\s*\(' src | sed -n '1,120p'
🧹 Nitpick comments (5)
src/stories/beginners/remove-points/index.ts (1)
25-61: Type the callback param and drop the redundant undefined guard; simplify link reindexing in one pass.The new API guarantees a point click, so (i) should be typed and not checked for undefined. Also, avoid the intermediate pairedNumbers array.
Apply:
- onPointClick: (i): void => { - if (i !== undefined) { + onPointClick: (i: number): void => { // Filter out the clicked point from positions array const currentPositions = graph.getPointPositions() - const newPointPositions = currentPositions - .filter((pos, posIndex) => { - return ( - (posIndex % 2 === 0 && posIndex !== i * 2) || - (posIndex % 2 === 1 && posIndex !== i * 2 + 1) - ) - }) + const newPointPositions = currentPositions.filter((_, posIndex) => + (posIndex % 2 === 0 && posIndex !== i * 2) || + (posIndex % 2 === 1 && posIndex !== i * 2 + 1) + ) - // Convert links array to source-target pairs for easier filtering - const pairedNumbers = [] - for (let j = 0; j < graphLinks.length; j += 2) { - const pair = [graphLinks[j], graphLinks[j + 1]] - pairedNumbers.push(pair) - } - - // Remove links connected to deleted point and adjust indices of remaining links - graphLinks = (pairedNumbers - .filter( - ([sourceIndex, targetIndex]) => sourceIndex !== i && targetIndex !== i - ) - .flat() as number[]) - .map((p) => { - if (p > i) return p - 1 - else return p - }) + // Remove links connected to the deleted point and reindex > i + const newLinks: number[] = [] + for (let j = 0; j < graphLinks.length; j += 2) { + const s = graphLinks[j] + const t = graphLinks[j + 1] + if (s !== i && t !== i) { + newLinks.push(s > i ? s - 1 : s, t > i ? t - 1 : t) + } + } + graphLinks = newLinks graph.setPointPositions(new Float32Array(newPointPositions)) graph.setLinks(new Float32Array(graphLinks)) graph.render(isPaused ? 0 : undefined) - } console.log('Clicked node: ', i) },src/stories/beginners/quick-start.ts (1)
21-22: LGTM; add explicit types for clarity.- onPointClick: pointIndex => { console.log('Clicked point index: ', pointIndex) }, + onPointClick: (pointIndex: number) => { console.log('Clicked point index: ', pointIndex) },src/stories/2. configuration.mdx (1)
19-19: Minor style: simplify wording.“Cursor style when hovering a link” reads tighter.
-| hoveredLinkCursor | Cursor style to use when hovering over a link | `auto` | +| hoveredLinkCursor | Cursor style when hovering a link | `auto` |src/index.ts (1)
1268-1276: Clear hoveredLinkIndex on update to avoid stale indices after data changes.Prevents transient stale hover when links array changes between frames.
this.create() this.initPrograms() this.store.hoveredPoint = undefined + this.store.hoveredLinkIndex = undefined this.start(simulationAlpha)src/config.ts (1)
367-400: Document onClick + specialized click interplay.Consider clarifying that onClick fires in addition to onPointClick/onLinkClick/onBackgroundClick to set expectations.
- * Callback function that will be called on every canvas click. + * Callback function that will be called on every canvas click (fires in addition to the specialized click callbacks).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
src/config.ts(8 hunks)src/index.ts(15 hunks)src/stories/1. welcome.mdx(1 hunks)src/stories/2. configuration.mdx(3 hunks)src/stories/beginners/basic-set-up/index.ts(2 hunks)src/stories/beginners/quick-start.ts(1 hunks)src/stories/beginners/remove-points/index.ts(1 hunks)src/stories/clusters/polygon-selection/index.ts(1 hunks)src/stories/shapes/image-example/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/stories/beginners/basic-set-up/index.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-25T11:35:22.327Z
Learnt from: Stukova
PR: cosmosgl/graph#173
File: package.json:3-3
Timestamp: 2025-08-25T11:35:22.327Z
Learning: In cosmos.gl/graph, when the getTrackedPointPositionsMap() method was changed from returning Map to ReadonlyMap, the team decided not to bump the major version because this breaking change only affects code that was mutating the returned Map (considered an anti-pattern), and provides performance and safety benefits. Users needing mutable access can create a copy with `new Map(graph.getTrackedPointPositionsMap())`.
Applied to files:
src/stories/clusters/polygon-selection/index.tssrc/stories/beginners/remove-points/index.ts
🧬 Code graph analysis (5)
src/stories/clusters/polygon-selection/index.ts (1)
src/stories/clusters/polygon-selection/polygon.ts (1)
PolygonSelection(3-143)
src/stories/beginners/quick-start.ts (1)
src/stories/create-cosmos.ts (1)
props(18-68)
src/stories/shapes/image-example/index.ts (1)
src/stories/beginners/point-labels/labels.ts (1)
update(14-45)
src/index.ts (6)
src/variables.ts (3)
defaultConfigValues(12-64)defaultGreyoutPointColor(3-3)defaultBackgroundColor(10-10)src/helper.ts (2)
getRgbaColor(16-32)readPixels(38-45)src/modules/Store/index.ts (3)
ALPHA_MIN(8-8)MAX_HOVER_DETECTION_DELAY(16-16)Store(13-142)src/modules/Drag/index.ts (1)
Drag(5-33)src/modules/Points/index.ts (1)
findHoveredPoint(723-726)src/modules/GraphData/index.ts (1)
GraphData(16-383)
src/config.ts (3)
src/modules/Store/index.ts (1)
Hovered(18-18)src/variables.ts (1)
defaultConfigValues(12-64)src/modules/GraphData/index.ts (2)
GraphData(16-383)updateArrows(270-282)
🪛 LanguageTool
src/stories/2. configuration.mdx
[grammar] ~19-~19: There might be a mistake here.
Context: ...use when hovering over a link | auto | | renderHoveredPointRing | Turns ring re...
(QB_NEW_EN)
[style] ~97-~97: This phrase is redundant. Consider writing “space”.
Context: ...oundClick | Called when the background (empty space) is clicked | | onMouseMove | Called on...
(EMPTY_HOLE)
🔇 Additional comments (15)
src/stories/1. welcome.mdx (2)
38-39: Docs update matches API rename.
38-39: Verified no legacyonClickhandlers in stories/docs — all story files and MDX useonPointClick/onBackgroundClickas intended; the only remainingonClickreference is the default insrc/config.ts.src/stories/clusters/polygon-selection/index.ts (1)
20-22: LGTM: background click cleanly unselects points.src/stories/shapes/image-example/index.ts (1)
197-204: LGTM: point and background selection handlers align with new API.src/index.ts (6)
274-286: Background color fallback — LGTM.Handles explicit undefined by falling back to defaultBackgroundColor.
286-288: Hovered link color propagation — LGTM.Defaults applied when unset; matches variables.ts.
314-318: Correct: toggling link-hovering based on callbacks.Wires enable/disable cleanly through the store.
1391-1406: Confirm intended onClick semantics (general + specialized).onClick fires always, then onPointClick/onLinkClick/onBackgroundClick fire conditionally. Confirm this double-dispatch is by design for API consumers.
1487-1508: Unified hover picking and correct index-0 handling — LGTM.Throttling, undefined checks, and ≥0 test for link index prevent the “index 0” bug and stale state.
Also applies to: 1541-1569
1434-1455: Resize-time FBO upkeep gated by feature flag — LGTM.src/config.ts (5)
101-106: New hoveredLinkCursor config — LGTM.
166-179: Hovered link color/width-increase options — LGTM.Docs and defaults align with variables.ts.
349-355: Introduce onSimulationUnpause (with deprecation note) — LGTM.
436-451: onLinkMouseOver/Out signatures — LGTM.Index for over; event for out matches usage in src/index.ts.
623-678: GraphConfig fields for link-hover features — LGTM.
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/stories/2. configuration.mdx (1)
94-103: Clarify callback parameter docs and remove “empty space”.Align with the actual API signatures and add MouseEvent/D3 event info; remove redundant wording.
-| onClick | Called on canvas click with point index and position | -| onPointClick | Called when a point is clicked | -| onLinkClick | Called when a link is clicked | -| onBackgroundClick | Called when the background (empty space) is clicked | -| onMouseMove | Called on mouse movement with hover info | +| onClick | Called on canvas click with point index (if any), point position (if any), and the MouseEvent | +| onPointClick | Called when a point is clicked (index, position, MouseEvent) | +| onLinkClick | Called when a link is clicked (linkIndex, MouseEvent) | +| onBackgroundClick | Called when the background is clicked (MouseEvent) | +| onMouseMove | Called on mouse movement with hover info and the MouseEvent | @@ -| onLinkMouseOver | Called when pointer enters a link | -| onLinkMouseOut | Called when pointer leaves a link | +| onLinkMouseOver | Called when pointer enters a link (linkIndex) | +| onLinkMouseOut | Called when pointer leaves a link (MouseEvent/D3ZoomEvent/D3DragEvent/undefined) |
🧹 Nitpick comments (3)
src/stories/2. configuration.mdx (1)
19-19: Minor grammar/style: end sentence with a period.-| hoveredLinkCursor | Cursor style to use when hovering over a link | `auto` | +| hoveredLinkCursor | Cursor style to use when hovering over a link. | `auto` |src/config.ts (2)
392-396: Tighten wording: drop “(empty space)”.- * Callback function that will be called when the background (empty space) is clicked. + * Callback function that will be called when the background is clicked.
436-450: Consider including the event parameter inonLinkMouseOverfor parity withonLinkMouseOut. If you add the second(event)argument to the signature, update the call in src/index.ts (line 1566) tothis.config.onLinkMouseOver?.(this.store.hoveredLinkIndex, this.currentEvent). Otherwise, add a JSDoc note ononLinkMouseOverexplaining why the event is intentionally omitted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/config.ts(8 hunks)src/modules/Lines/draw-curve-line.vert(6 hunks)src/modules/Store/index.ts(3 hunks)src/stories/2. configuration.mdx(3 hunks)src/variables.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/modules/Lines/draw-curve-line.vert
- src/modules/Store/index.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/variables.ts (3)
src/modules/GraphData/index.ts (1)
updateArrows(270-282)src/modules/Lines/index.ts (7)
config(67-67)updateArrow(151-155)config(74-74)config(78-78)config(66-66)config(76-76)config(71-71)src/index.ts (1)
setLinkArrows(496-500)
src/config.ts (2)
src/modules/Store/index.ts (1)
Hovered(18-18)src/variables.ts (1)
defaultConfigValues(12-64)
🪛 LanguageTool
src/stories/2. configuration.mdx
[grammar] ~19-~19: There might be a mistake here.
Context: ...use when hovering over a link | auto | | renderHoveredPointRing | Turns ring re...
(QB_NEW_EN)
[style] ~97-~97: This phrase is redundant. Consider writing “space”.
Context: ...oundClick | Called when the background (empty space) is clicked | | onMouseMove | Called on...
(EMPTY_HOLE)
🔇 Additional comments (9)
src/variables.ts (3)
27-27: No changes required for hoveredLinkCursor usage. The updateCanvasCursor method already applies hoveredLinkCursor when isLinkHoveringEnabled and hoveredLinkIndex is set.
17-17: Rename verified: no stale identifiers remain
Search across.ts/.tsxfiles yielded no matches forarrowSizeScaleorarrowLinks.
30-31: Hovered link visuals wired into rendering path
Lines module consumeshoveredLinkColorandhoveredLinkWidthIncreasefrom the store and config and passes them into the draw call, confirming the rendering path applies hovered link styling.src/stories/2. configuration.mdx (2)
30-31: Docs for hovered link visuals: LGTM.
92-93: Deprecation note and new callback: LGTM.src/config.ts (4)
101-106: NewhoveredLinkCursor: LGTM.
346-354: Deprecation + newonSimulationUnpause: LGTM.
167-179: Approve: hovered link styling is wired end-to-end and ready for use.
367-399: Ignore this comment: click callbacks are already wired
onPointClick, onLinkClick, and onBackgroundClick are invoked in src/index.ts (lines 1392–1406), so no further action is needed.Likely an incorrect or invalid review comment.
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Stukova
changed the title
Fixed with help of coderabbit. Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (12)
src/stories/beginners/link-hovering/data-generator.ts (3)
100-101: Deduplicate connections across strategies (treat as undirected).Prevents duplicate links from different phases which can bloat buffers and skew visuals.
- return connections + // Deduplicate + const seen = new Set<string>() + const unique: Link[] = [] + for (const { source, target } of connections) { + const a = Math.min(source, target) + const b = Math.max(source, target) + const key = `${a}-${b}` + if (!seen.has(key)) { + seen.add(key) + unique.push({ source, target }) + } + } + return unique
21-42: Clamp HSL→RGB outputs to [0,1].Avoids float drift outside [0,1] due to rounding.
- return [r + m, g + m, b + m] + const rm = r + m, gm = g + m, bm = b + m + return [ + Math.min(1, Math.max(0, rm)), + Math.min(1, Math.max(0, gm)), + Math.min(1, Math.max(0, bm)), + ]
103-116: Optional: deterministic jitter for reproducible demos.If you want consistent story visuals across hot-reloads, inject an RNG (seeded) instead of Math.random, e.g., pass rng?: () => number into generatePointPositions and generateConnections.
src/stories/beginners/link-hovering/index.ts (2)
23-23: Add rel to external link for security.Prevent reverse tabnabbing on target="_blank".
- attribution: 'visualized with <a href="https://cosmograph.app/" style="color: var(--cosmosgl-attribution-color);" target="_blank">Cosmograph</a>', + attribution: 'visualized with <a href="https://cosmograph.app/" style="color: var(--cosmosgl-attribution-color);" target="_blank" rel="noopener noreferrer">Cosmograph</a>',
16-23: Showcase new config flags in the demo (cursor/color).Optional but helps users discover the new options introduced by this PR.
const config: GraphConfigInterface = { backgroundColor: '#2d313a', scalePointsOnZoom: true, linkArrows: false, curvedLinks: true, enableSimulation: false, hoveredLinkWidthIncrease: 4, + hoveredLinkCursor: 'pointer', + hoveredLinkColor: '#ffd54a', attribution: 'visualized with <a href="https://cosmograph.app/" style="color: var(--cosmosgl-attribution-color);" target="_blank">Cosmograph</a>',src/stories/beginners/link-hovering/style.css (7)
3-7: Consider theming and containment for the container.
- Use CSS variables for the gradient colors to align with the tokens you added in :root.
- Add
contain: paint;to limit paint scope since overflow is hidden and background is heavy.Apply:
.company-network { - background: radial-gradient(circle at 50% 50%, #1a1a2e 0%, #0a0a0a 100%); + background: radial-gradient(circle at 50% 50%, var(--cosmos-dark-bg, #1a1a2e) 0%, #0a0a0a 100%); position: relative; overflow: hidden; + contain: paint; }
9-12: Avoidtransition: alland improve Safari support for blur.
- Limit transitioned properties for perf.
- Add
-webkit-backdrop-filterand a translucent fallback background for non-supporting browsers..info-panel { - backdrop-filter: blur(12px); - transition: all 0.3s cubic-bezier(0.4, 0, 0.2, 1); + background-color: color-mix(in srgb, var(--cosmos-dark-bg, rgba(16, 20, 40, 0.95)) 85%, transparent); + backdrop-filter: blur(12px); + -webkit-backdrop-filter: blur(12px); + transition: transform 0.3s cubic-bezier(0.4, 0, 0.2, 1), box-shadow 0.3s cubic-bezier(0.4, 0, 0.2, 1); + will-change: transform; }
14-17: Respect reduced motion preferences.Provide a no-motion path to avoid hover animations for users with motion sensitivity.
.info-panel:hover { transform: translateY(-2px); box-shadow: 0 20px 48px rgba(0, 0, 0, 0.6), 0 8px 24px rgba(0, 0, 0, 0.4); } +@media (prefers-reduced-motion: reduce) { + .info-panel { transition: none; } + .info-panel:hover { transform: none; } +}
19-25: Internationalization-friendly spacing.Prefer logical properties so indicators place correctly in RTL.
.relationship-type-indicator { display: inline-block; width: 12px; height: 12px; border-radius: 50%; - margin-right: 8px; + margin-inline-end: 8px; }
27-45: Deduplicate hardcoded colors with variables.Map variant fills/shadows to the tokens you defined to keep theme changes centralized.
.relationship-type-indicator.management { - background: linear-gradient(45deg, #FFAD6B, #FF8A65); - box-shadow: 0 2px 8px rgba(255, 173, 107, 0.4); + background: linear-gradient(45deg, var(--cosmos-orange, #FFAD6B), #FF8A65); + box-shadow: 0 2px 8px color-mix(in srgb, var(--cosmos-orange, #FFAD6B) 40%, transparent); } .relationship-type-indicator.collaboration { - background: linear-gradient(45deg, #59C0FF, #42A5F5); - box-shadow: 0 2px 8px rgba(89, 192, 255, 0.4); + background: linear-gradient(45deg, var(--cosmos-blue, #59C0FF), #42A5F5); + box-shadow: 0 2px 8px color-mix(in srgb, var(--cosmos-blue, #59C0FF) 40%, transparent); } .relationship-type-indicator.mentorship { - background: linear-gradient(45deg, #6BD17B, #66BB6A); - box-shadow: 0 2px 8px rgba(107, 209, 123, 0.4); + background: linear-gradient(45deg, var(--cosmos-green, #6BD17B), #66BB6A); + box-shadow: 0 2px 8px color-mix(in srgb, var(--cosmos-green, #6BD17B) 40%, transparent); } .relationship-type-indicator.friendship { - background: linear-gradient(45deg, #FF61AD, #EC407A); - box-shadow: 0 2px 8px rgba(255, 97, 173, 0.4); + background: linear-gradient(45deg, var(--cosmos-pink, #FF61AD), #EC407A); + box-shadow: 0 2px 8px color-mix(in srgb, var(--cosmos-pink, #FF61AD) 40%, transparent); }
47-55: Tighten transitions and use logical border for RTL.
- Limit transitioned properties.
- Use
border-inline-startoverborder-left..person-card { background: rgba(255, 255, 255, 0.03); border-radius: 8px; padding: 12px; margin: 6px 0; - border-left: 3px solid transparent; + border-inline-start: 3px solid transparent; border: 1px solid rgba(255, 255, 255, 0.06); - transition: all 0.2s ease; + transition: box-shadow 0.2s ease, background-color 0.2s ease, border-color 0.2s ease; }
57-61: Tokenize highlight colors.Leverage
--cosmos-blueto align with theme and ease updates..person-card.highlighted { - border-left-color: #59C0FF; - background: rgba(89, 192, 255, 0.08); - box-shadow: 0 4px 16px rgba(89, 192, 255, 0.15); + border-inline-start-color: var(--cosmos-blue, #59C0FF); + background: color-mix(in srgb, var(--cosmos-blue, #59C0FF) 8%, transparent); + box-shadow: 0 4px 16px color-mix(in srgb, var(--cosmos-blue, #59C0FF) 15%, transparent); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/stories/beginners.stories.ts(3 hunks)src/stories/beginners/link-hovering/data-generator.ts(1 hunks)src/stories/beginners/link-hovering/index.ts(1 hunks)src/stories/beginners/link-hovering/style.css(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/stories/beginners.stories.ts (2)
src/stories/create-story.ts (4)
Story(5-5)createStory(7-51)story(32-38)storyFunction(15-51)src/stories/beginners/link-hovering/index.ts (1)
linkHovering(5-61)
src/stories/beginners/link-hovering/index.ts (1)
src/stories/beginners/link-hovering/data-generator.ts (1)
generateData(178-198)
src/stories/beginners/link-hovering/data-generator.ts (2)
src/stories/generate-mesh-data.ts (1)
generateMeshData(30-125)src/modules/GraphData/index.ts (1)
GraphData(16-383)
🔇 Additional comments (8)
src/stories/beginners/link-hovering/data-generator.ts (2)
140-176: LGTM: link buffers and per-link attributes are sized and filled correctly.Shapes match GraphData expectations (2 indices per link; RGBA per link; width per link).
178-198: LGTM: returned data structure aligns with GraphData contract.All required typed arrays provided with expected lengths.
src/stories/beginners.stories.ts (3)
9-9: LGTM: story import added.Import path and usage look consistent with the other beginner stories.
23-25: LGTM: sourceCode raw imports.Good inclusion for Storybook code panels.
104-114: LGTM: LinkHovering story registration.Name, createStory usage, and sourceCode panel entries look correct.
src/stories/beginners/link-hovering/index.ts (2)
38-44: LGTM: data binding sequence.All required buffers are set before render.
25-33: Confirm callback signature matches GraphConfigInterface
I wasn’t able to locate the interface definition—please verify that onLinkMouseOut’s handler signature includes an optional MouseEvent parameter (e.g.(e?: MouseEvent) => void), since the library invokesonLinkMouseOut(event)and strictFunctionTypes requires matching parameters.src/stories/beginners/link-hovering/style.css (1)
63-73: Great to see centralized tokens. Consider using them or trimming.You define tokens but most rules still use literals. Either refactor to consume them (above diffs) or keep only those actively used to avoid drift.
Do you want me to sweep the story folder and replace literal colors with these tokens?
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/stories/beginners/remove-points/index.ts (1)
25-35:iis guaranteed (onPointClick), previous concern resolvedWith onPointClick, the index parameter is always a number; the earlier concern about
ipossibly being undefined no longer applies.
🧹 Nitpick comments (3)
src/config.ts (2)
436-451: Make link hover callbacks symmetrical and carry event contextonLinkMouseOver currently exposes only linkIndex, while onLinkMouseOut exposes only the event. To align with the point events and provide useful context (modifiers, etc.), pass the event to onLinkMouseOver as well.
Apply:
/** * Callback function that will be called when the mouse moves over a link. - * The link index will be passed as the first argument: - * `(linkIndex: number) => void`. + * The link index will be passed as the first argument and the event as the second: + * `(linkIndex: number, event: MouseEvent | D3ZoomEvent<HTMLCanvasElement, undefined> | D3DragEvent<HTMLCanvasElement, undefined, Hovered> | undefined) => void`. * Default value: `undefined` */ - onLinkMouseOver?: (linkIndex: number) => void; + onLinkMouseOver?: ( + linkIndex: number, + event: MouseEvent | D3ZoomEvent<HTMLCanvasElement, undefined> | D3DragEvent<HTMLCanvasElement, undefined, Hovered> | undefined + ) => void;
367-379: Deprecate onClick in docs — Graph still emits both onClick and the granular callbacksAdd an @deprecated note to the onClick doc in src/config.ts (onClick doc ~lines 360–366). src/index.ts:onClick (~lines 1384–1406) invokes config.onClick then calls onPointClick / onLinkClick / onBackgroundClick.
/** - * Callback function that will be called on every canvas click. + * Callback function that will be called on every canvas click. + * @deprecated Use `onPointClick`, `onLinkClick`, and `onBackgroundClick` instead. * If clicked on a point, its index will be passed as the first argument, * position as the second argument and the corresponding mouse event as the third argument: * `(index: number | undefined, pointPosition: [number, number] | undefined, event: MouseEvent) => void`. * Default value: `undefined` */src/stories/beginners/remove-points/index.ts (1)
36-53: Avoid intermediate arrays; do link filtering+reindexing in one passThis reduces allocations and improves clarity.
- // Convert links array to source-target pairs for easier filtering - const pairedNumbers = [] - for (let j = 0; j < graphLinks.length; j += 2) { - const pair = [graphLinks[j], graphLinks[j + 1]] - pairedNumbers.push(pair) - } - - // Remove links connected to deleted point and adjust indices of remaining links - graphLinks = (pairedNumbers - .filter( - ([sourceIndex, targetIndex]) => sourceIndex !== i && targetIndex !== i - ) - .flat() as number[]) - .map((p) => { - if (p > i) return p - 1 - else return p - }) + // Remove links connected to deleted point and adjust indices of remaining links + const newLinks: number[] = [] + for (let j = 0; j < graphLinks.length; j += 2) { + const s = graphLinks[j] + const t = graphLinks[j + 1] + if (s !== i && t !== i) { + newLinks.push(s > i ? s - 1 : s, t > i ? t - 1 : t) + } + } + graphLinks = newLinks
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/config.ts(8 hunks)src/stories/2. configuration.mdx(3 hunks)src/stories/beginners/remove-points/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/stories/2. configuration.mdx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-25T11:35:22.327Z
Learnt from: Stukova
PR: cosmosgl/graph#173
File: package.json:3-3
Timestamp: 2025-08-25T11:35:22.327Z
Learning: In cosmos.gl/graph, when the getTrackedPointPositionsMap() method was changed from returning Map to ReadonlyMap, the team decided not to bump the major version because this breaking change only affects code that was mutating the returned Map (considered an anti-pattern), and provides performance and safety benefits. Users needing mutable access can create a copy with `new Map(graph.getTrackedPointPositionsMap())`.
Applied to files:
src/stories/beginners/remove-points/index.ts
🧬 Code graph analysis (2)
src/stories/beginners/remove-points/index.ts (1)
src/index.ts (1)
onClick(1335-1341)
src/config.ts (5)
src/modules/Store/index.ts (1)
Hovered(18-18)src/variables.ts (1)
defaultConfigValues(12-64)src/index.ts (3)
GraphConfigInterface(1605-1605)setConfig(226-288)Graph(26-1483)src/modules/GraphData/index.ts (3)
GraphData(16-383)updateArrows(270-282)updateLinkColor(217-243)src/modules/Drag/index.ts (1)
Drag(5-33)
🔇 Additional comments (3)
src/config.ts (2)
624-624: Config surface additions look consistent with defaults and data pathsDefaults map to defaultConfigValues and renamed linkArrows/linkArrowsSizeScale align with GraphData.updateArrows expectations.
Also applies to: 634-636, 641-642, 667-667, 670-673, 676-677
101-106: No action needed — link-hover UX is wired correctlyupdateCanvasCursor reads config.hoveredLinkCursor and sets the cursor for hovered links; hoveredLinkColor is stored via setHoveredLinkColor and hoveredLinkWidthIncrease + hoveredLinkColor are passed to the Lines module and applied in the shader (draw-curve-line.vert). See src/index.ts:updateCanvasCursor, src/modules/Store/index.ts:setHoveredLinkColor, and src/modules/Lines/{index.ts, draw-curve-line.vert}.
src/stories/beginners/remove-points/index.ts (1)
54-56: Confirm render signature when pausedFile: src/stories/beginners/remove-points/index.ts (lines 54–56)
graph.setPointPositions(new Float32Array(newPointPositions)) graph.setLinks(new Float32Array(graphLinks)) graph.render(isPaused ? 0 : undefined)Passing 0 assumes render(alpha?: number); verify Graph.render's signature and behavior for alpha === 0 — if 0 is treated as falsy/causes a no-op, call graph.render() or graph.render(undefined) (or update render to accept 0 explicitly).
2 participants
New Features:
onLinkMouseOver,onLinkMouseOut,onLinkClick,onPointClick,onBackgroundClickhoveredLinkColor,hoveredLinkCursor,hoveredLinkWidthIncreaseunpause()(replaces deprecatedrestart())Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation