A Collection of Fixes - #250
Open
Stukova wants to merge 19 commits into
Open
Conversation
📝 WalkthroughWalkthroughGraph runtime ownership is scoped per instance. Data-texture reads use integer texel addressing. Point and link validation is stricter. Link rendering supports blended and opaque modes. Lifecycle and configuration behavior are documented and refreshed. ChangesData texture addressing and runtime updates
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Graph
participant ForcePipelines
participant PointAndLinePipelines
participant DataTextures
Graph->>ForcePipelines: Run force and cluster passes
Graph->>PointAndLinePipelines: Run rendering and selection passes
ForcePipelines->>DataTextures: Fetch indexed texels with texelFetch
PointAndLinePipelines->>DataTextures: Fetch indexed texels with texelFetch
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/stories/api-reference.mdxParsing error: ESLint was configured to run on
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/FPSMonitor/index.ts`:
- Around line 7-15: Update the FPSMonitor cleanup logic used by the constructor
and destroy method to query only direct children of this.container, using scoped
selectors for `#gl-bench` and `#gl-bench-style`. Preserve removal of this monitor’s
own elements while preventing nested containers from deleting an ancestor
monitor.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d6b15d7-6b4b-4f0b-9e2d-9688f8ee527c
📒 Files selected for processing (4)
src/helper.tssrc/index.tssrc/modules/FPSMonitor/index.tssrc/modules/Store/index.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/Zoom/index.ts`:
- Around line 17-20: Update the disabled-zoom filter in the Zoom module to also
reject touchend events, preventing d3-zoom’s double-tap path when enableZoom is
false. Add a regression test covering double-tap behavior and verify no scale
transition occurs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 17e90fcb-eec2-43b9-8297-b624d74a5ec7
📒 Files selected for processing (2)
src/index.tssrc/modules/Zoom/index.ts
Three shared-global collisions broke pages hosting more than one Graph: - The space-key handlers were registered on document under the fixed d3 namespace .cosmos, so a second instance silently replaced the first instance's handlers, and either instance's destroy() removed the survivor's. Each instance now namespaces its document listeners with a random id (.cosmos-<id>). - The --cosmosgl-attribution-color / --cosmosgl-error-message-color CSS variables were written to document.documentElement, making instances with different backgrounds fight over one global value. They are now set on the graph's container div, which the attribution and error elements inherit from. - The FPS monitor widget and its injected style lived on document.body under the global ids #gl-bench / #gl-bench-style; constructing a second monitor removed the first instance's widget, and Graph's destroy() reached into gl-bench internals. The widget is now mounted inside the graph container and cleaned up by FPSMonitor.destroy(), which also fixes the style element leaking on every showFPSMonitor toggle. Any number of Graph instances can now coexist on one page - and be destroyed in any order - without affecting each other's key handling, attribution contrast, or FPS monitor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Scoping the gl-bench cleanup to the graph's container (ca21698) still used descendant queries, so a monitor whose container encloses another graph's container — e.g. one graph on document.body and another mounted inside it — could find and remove the nested monitor's widget and style on construct or destroy. - Use ':scope >' selectors: gl-bench appends both #gl-bench and #gl-bench-style as direct children of the dom it is given, so the direct-child query always reaches the monitor's own elements and never a nested instance's. A monitor now removes only elements it created, regardless of how graph containers nest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Three config properties accepted new values via setConfig without any effect: - pointSamplingDistance / linkSamplingDistance were only read when the sampling grids were rebuilt on canvas resize or data recreation, so a runtime change did nothing until the window resized. The config diff now rebuilds the grids; the rebuild is idempotent and skips when the grid dimensions are unchanged. - pointDefaultSize left point image sizes stale forever: they default to a copy of point sizes, but the config branch only refreshed the size channel. It now refreshes image sizes too; explicit user-set image sizes are preserved since only missing/NaN entries resolve through the default. - enableZoom: false only detached wheel.zoom, leaving double-click and pinch zoom active. A d3-zoom filter now blocks the scale-changing gestures (wheel, dblclick, multi-touch) while keeping panning and programmatic zoom alive. setConfig now guarantees these keys take effect immediately, matching every other config property. Co-authored-by: Nikita Rokotyan <nikita@rokotyan.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Ten fetches in the force shaders addressed their data textures at the texel CORNER, `index / textureSize`. That coordinate lands exactly on the boundary between texel index-1 and index, and NEAREST selection is a floor of the size-scaled coordinate, so it returns the intended texel only while the driver's arithmetic does not fall even one ULP short. It falls short constantly. Measured on an Apple M3 through ANGLE Metal, 2284 of the 4095 texture sizes from 2 to 4096 misfetch at least one index; at size 100, 3916 of 10000 texels read their neighbour. Only powers of two are immune, and pointsTextureSize / clustersTextureSize are ceil(sqrt(count)) — so roughly half of all point counts land on a size that silently reads the wrong point. Which sizes fail is a driver property, not an arithmetic one: SwiftShader's failing set is nearly disjoint from Metal's, so no texture size is portably safe. The engine's own writes never had this problem — every pass rasterises to texel centres, `2.0 * (index + 0.5) / size - 1.0`. It was only the reads that failed to invert them. The contract is now uniform: a data texture is an array, so it is addressed by index. `texelFetch` takes the integer texel directly, does no coordinate arithmetic, and ignores filter and wrap state, which removes the defect instead of hiding it behind a margin. `texture()` survives only where the coordinate is genuinely continuous — the image atlas, the one place filtering is the point. - Clusters/force-cluster.frag is where users saw it. The cluster force's entire target is one fetch shared by every member of a cluster, so a misfetch relocates a whole cluster onto its neighbour. With 1089 pinned clusters (clustersTextureSize 33) only 7.1% of points reached their own cluster; they now all do, and a control at the exact size 23 is unchanged in both builds. - The exit-status reads convert together with the position reads beside them. They shared one coordinate expression, so they erred together and the shader coherently processed the wrong point; converting only the positions would let point k's NaN position past point k-1's absence guard and poison the centroid and collision sums. - The reads that already used the `(index + 0.5) / size` centre form convert too. They were correct, but only because half a texel of margin absorbed the same driver error — one rule is worth more than a second form that has to be re-justified at every new call site. - Full-screen passes take their texel from `ivec2(gl_FragCoord.xy)` instead of an interpolated quad varying, so each fragment addresses its own element exactly rather than by a rasterised coordinate. Every such pass renders into a target whose dimensions equal the textures it samples. quad.vert's varying had no consumer left and is gone. - ForceLink/force-spring.ts is included. Its shader is built from a template literal, so it is invisible to a sweep filtered to .vert/.frag files — sweep for `#version 300 es` instead. - find-points-in-polygon.frag asks `textureSize()` for its path texture's width rather than re-deriving ceil(sqrt(pathLength)), which duplicated the formula the allocation used and shadowed that builtin. - draw-highlighted.vert guards its index before the integer `%` and `/`. `pointIndex` defaults to -1, and both operators are undefined on a negative or zero operand where the old float `mod()` was not. - Nine uniform-block members lost their last reader and are removed, taking two whole UniformStores with them. A member spans the std140 block, its `#define`, the non-UBO declaration and three TypeScript sites, and nothing checks that they agree, so each was removed in lockstep and every block's order re-verified against its uniformTypes. Two consequences worth knowing. The 1×1 all-zero exit texture bound when no point is absent now relies on WebGL 2 defining an out-of-range texelFetch as zero, where it previously relied on CLAMP_TO_EDGE; the optimisation is documented at the allocation. And trackPointPositions never re-bakes its texel pairs when the point count changes, so a stale tracked index now reports (0, 0) instead of another point's position — pre-existing, but the symptom changed and it wants its own fix. A data-texture read can no longer resolve to the wrong element. Behaviour is otherwise unchanged: everything that was already correct produces identical positions, identical rect, polygon and sampling results, and a byte-identical frame. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Captures the reasoning behind `fix(shaders): read data textures by texel index, never by coordinate` (f6c6a97): the measurements that showed corner addressing failing on real hardware, and the two alternatives that were weighed and rejected — adding the `(index + 0.5)` half-texel margin, which only hides the fragility and has to be re-argued at every new call site, and switching the size uniforms to `textureSize()`, which cannot answer for a render target and misreports a bound placeholder. Also records what the change quietly moved: the 1×1 exit-texture stand-in now rests on WebGL defining an out-of-range fetch as zero rather than on CLAMP_TO_EDGE, which supersedes the justification given in the NaN point-removal entry, and the tracked-index staleness whose symptom shifted without its cause being fixed. The verification table states the evidence and, next to it, that all of it comes from a single GPU. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Stukova Olya <stukova.o@gmail.com>
The disabled-zoom filter rejected wheel, dblclick, and multi-touch, but a single-finger double-tap still zoomed: d3-zoom implements double-tap by rerouting the second tap's touchend into its dblclick handler, which re-applies the filter with that touchend event — a type the filter let through, so the ×2 scale transition ran anyway. - Reject touchend in the disabled branch. d3-zoom consults the filter only from its wheel, mousedown, dblclick, and touchstart handlers, so a touchend reaches it solely via the double-tap reroute — rejecting it cannot affect one-finger panning. - Verified in headless Chromium with touch against real Graph instances: the pre-fix bundle zooms 1 → 2 on double-tap despite enableZoom: false; the fixed bundle holds 1, double-tap still zooms when enabled, and one-finger panning keeps working while disabled. Disabling zoom now blocks every scale-changing gesture — wheel, double-click, pinch, and double-tap — while panning and programmatic zoom stay live. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Stukova
force-pushed
the
fix/multi-instance-globals
branch
from
August 4, 2026 12:40
9a9a772 to
80505e9
Compare
Two caller-supplied array shapes could break the layout, both of them values a user can produce without doing anything obviously wrong. - A NaN or negative value in setLinkStrength slipped past the `??` fallback in ForceLink, so Math.sqrt wrote NaN into the strength texture. The poisoned velocity reached update-position.frag, where clamp() turns NaN into 0 — both endpoints of that link snapped to (0, 0) and stuck there while the rest of the graph laid out normally. Invalid values now resolve to the degree-based default — the same fallback a missing value already used. - An odd-length setPointPositions array left a dangling x with no y, so pointsNumber came out fractional and new Array(pointsNumber) in the adjacency and degree builds threw RangeError: Invalid array length. The trailing value is now dropped with a warning before anything derives a count from it; subarray() is a view, so the caller's array is never edited. Verified on ANGLE/Apple M3: a NaN or negative strength on one link previously pinned its endpoints to (0, 0) and now lets them settle with the rest; an odd-length array previously threw and now yields the even prefix. Link rendering is unchanged. An invalid value in an input array now degrades to the documented default instead of corrupting positions the user never fed in. Co-authored-by: Nikita Rokotyan <nikita@rokotyan.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Stukova Olya <stukova.o@gmail.com>
The position texture is square, so any point count that is not a perfect square leaves trailing texels that hold position (0, 0) — a real location, the corner of the space. The search shaders run over every texel and cannot tell padding from a point, so each padding texel reports itself as found whenever the search area covers the screen position of the space origin. findPointsInRect and findPointsInPolygon then returned indices >= pointsNumber: points the caller never added. - extractIndicesFromPixels stops at the real point count. A texel's linear index is the point index (x = i % size, y = i / size), so padding is always the trailing run and truncating there cannot drop a real point. - The bound is an optional parameter. The helper is re-exported from the package entry, so external callers keep the current behaviour. Reachable without doing anything unusual: with 5 points, zooming out until the space corner is on screen and dragging a selection across the canvas returned 9 indices, 4 of which did not exist; it now returns 5, and a search over the origin returns none instead of 4. Perfect-square point counts have no padding and never failed, which made this look intermittent. A search result now holds only indices the caller can look up. Co-authored-by: Nikita Rokotyan <nikita@rokotyan.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/helper.ts (1)
91-100: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse fixed-width base-36 encoding in
generateRandomId.
generateRandomId()currently encodes eachUint32withword.toString(36)and joins them.Array.from([1n, 37n], n => n.toString(36)).join('')andArray.from([37n, 1n], n => n.toString(36)).join('')both produce"111". This namespace collision can make distinctGraphinstances share document-level event handlers. Pad each component before concatenation, for exampleword.toString(36).padStart(7, '0').🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/helper.ts` around lines 91 - 100, Update generateRandomId so each Uint32 word is encoded as a fixed-width base-36 segment before joining. Pad every word’s string representation to the maximum required width with leading zeroes, preserving unambiguous boundaries between the two random values.
🧹 Nitpick comments (1)
src/helper.ts (1)
111-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd boundary tests for
pointsNumber.Cover
undefined,0, a bound smaller than the available texels, and a bound larger than the buffer. Assert the exact returned indices from a valid RGBA fixture.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/helper.ts` around lines 111 - 115, In extractIndicesFromPixels, add boundary tests using a valid RGBA Float32Array fixture for pointsNumber undefined, 0, less than the available texels, and greater than the buffer length. Assert the exact returned indices for each case, including that the bound never causes out-of-range processing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/helper.ts`:
- Around line 91-100: Update generateRandomId so each Uint32 word is encoded as
a fixed-width base-36 segment before joining. Pad every word’s string
representation to the maximum required width with leading zeroes, preserving
unambiguous boundaries between the two random values.
---
Nitpick comments:
In `@src/helper.ts`:
- Around line 111-115: In extractIndicesFromPixels, add boundary tests using a
valid RGBA Float32Array fixture for pointsNumber undefined, 0, less than the
available texels, and greater than the buffer length. Assert the exact returned
indices for each case, including that the bound never causes out-of-range
processing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 53218444-7ada-4bbf-9ec1-bc0aa2e33214
📒 Files selected for processing (2)
src/helper.tssrc/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index.ts
Collision silently failed to separate points at ordinary sizes. The grid
cell measured one effective radius, but two touching points interact at
two radii apart and the 3x3 neighbourhood scan reaches only one cell of
separation — so a colliding pair could sit in cells the scan never
compares. The 8-unit cell floor hid it: points at the default size of 4
were covered, anything larger was not.
- The cell now spans the full interaction range, 2 x effectiveRadius.
The four offset passes reshuffle cell alignment to catch boundary
cases; they never extended the search radius, so no number of passes
could have closed this gap.
- A cell's accumulated position and size included the point itself while
the force count excluded it, dragging the average toward the point and
reporting a short distance — halved for a two-point cell, overstating
the overlap. The self-contribution is subtracted before averaging, and
a cell holding only this point is skipped.
Measured on settled simulations with collision as the only active force,
counting pairs left closer than their touching distance:
200 points, size 30 79 overlapping pairs, worst 24% of a diameter
interpenetrating -> 0
40 points, size 100 18 pairs, worst 18% -> 0
300 points, size 8 0 -> 0; the small-point case the cell floor
already covered is not degraded by wider cells
Collision resolves overlaps at any point size now, not only where the
cell floor happened to span the interaction range.
Co-authored-by: Nikita Rokotyan <nikita@rokotyan.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/ForceCollision/index.ts`:
- Around line 79-83: Update the cell-size calculation in the ForceCollision
initialization so the final value remains at least 2 * effectiveRadius after the
32-cell minimum and Math.ceil adjustments. Choose the grid dimension or clamp
the resulting cellSize to preserve this invariant, keeping the shader’s
adjacent-cell search unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f9c8f8d9-e685-4b2c-b2a8-9cdae2bf907a
📒 Files selected for processing (2)
src/modules/ForceCollision/force-collision-spatial.fragsrc/modules/ForceCollision/index.ts
`generateRandomId` concatenated two base-36 uint32 words without a fixed boundary, so distinct word pairs could produce the same id — (1, 1261) and (71, 1) both encode to "1z1". The id namespaces the `document` listeners each Graph instance registers, and a shared id is exactly the condition where instances remove each other's handlers. Each word is now padded to the 7 base-36 digits a full uint32 needs (36^6 < 2^32 < 36^7), which makes the encoding injective: every id is 14 characters and splits at a fixed offset. Scope, measured rather than assumed: with crypto-random 32-bit words both halves are 6-7 digits, so the ambiguity was nearly unreachable — 400,000 generated ids collided zero times and the lost entropy was 0.08 bits of 64. This buys correctness by construction, not a fix for observed breakage. Verified after the change: 200,006 pairs including both extremes round-trip exactly, all 14 characters, no collisions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Stukova Olya <stukova.o@gmail.com>
The cell was sized to the full interaction range and then immediately undone. Fitting it to a whole number of grid cells rounded the grid dimension *up*, which divides the space into cells slightly smaller than requested, and a 32-cell minimum pinned the cell at spaceSize / 32 no matter how large the radius grew — so above an effective radius of 64 (point size 128 at the default space size) the coverage gap the previous commit closed reopened completely. - The grid dimension now rounds down. Fitting can only grow the cell, so the range the shader's adjacent-cell search relies on always survives it. The shader is untouched. - The lower clamp drops from 32 cells to 1. A large radius legitimately wants a coarse grid; refusing to go below 32 was refusing the cell size the physics asks for. At one cell every point shares it and all pairs are still compared, which is the degenerate case where the interaction range covers the whole space. Measured on settled simulations with collision as the only active force: 30 points at size 300 left 26 overlapping pairs, worst 22% of a diameter interpenetrating, and now leave none. Sizes 8, 30 and 100 stay at zero, so nothing that already worked is disturbed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Stukova Olya <stukova.o@gmail.com>
… strokes With `linkBlending: false`, soft AA fringes still wrote full RGB with partial alpha. Canvas compositing treats that as premultiplied, so edges read brighter than the solid core (tubular / outlined look). A naive hard cut at `coverage < 0.5` would "fix" the halo but erase thin links — when stroke width ≤ the AA kernel, the descending `smoothstep` only peaks around `0.5` at the centerline, so that threshold discards almost every fragment (visible in the hyperbolic large-graph story with `linkDefaultWidth: 0.5`). **What changed:** The link fragment shader splits geometric `coverage` from color alpha, passes `linkBlending` as a trailing fragment uniform, and when blending is off discards only zero-coverage / fully transparent fragments then writes `vec4(color, 1.0)`. Blended and picking paths are unchanged. Signed-off-by: Nikita Rokotyan <nikita@rokotyan.com>
rokotyan
force-pushed
the
fix/multi-instance-globals
branch
from
August 4, 2026 21:09
e7a1c04 to
5acb03d
Compare
An odd-length links array made `linksNumber` fractional, and `new Array(linksNumber)` in updateArrows threw from inside the deferred render — the canvas went blank, points included, every public getter threw afterwards, and nothing reached the console. Endpoints themselves were never checked either: out-of-range, negative, fractional and NaN values reached the adjacency lists and the GPU untouched, so getNeighboringPointIndices reported points the caller cannot look up (`[99]`, `[-5]`, `[1.5]`) and a bogus link was drawn to whatever texel the index happened to address. Links now hold whole [source, target] pairs of real point indices. - The odd trailing value is dropped up front. `subarray` is a view, so the caller's array is not edited, and the check is a length test — no scan of the data. - Endpoints are validated where the links are already walked: the adjacency build skips an invalid pair, and the Lines endpoint buffer collapses one onto a single texel, which renders nothing (a zero-length link draws no pixels). Neither adds a pass. - Invalid links are neutralised in place, never removed. Link indices are part of the public API — onLinkClick, onLinkMouseOver, focusedLinkIndex, highlightedLinkIndices — and compacting the array would silently renumber every link after a dropped one. - `isPointIndex` replaces the same range test hand-written in getNeighboringPointIndices, getConnectedLinkIndices and getPointRadiusByIndex, which now also reject a fractional index instead of quietly returning nothing. - `pair()` had the same fractional-length flaw and threw a RangeError on an odd array; it now drops the unpaired trailing value. Verified: an odd links array renders exactly as the even prefix instead of blanking the canvas; out-of-range, negative, fractional and NaN endpoints each report no neighbours and draw zero pixels while the valid links in the same array are untouched; and a valid link *after* an invalid one keeps its original index. Co-authored-by: Nikita Rokotyan <nikita@rokotyan.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/GraphData/index.ts`:
- Around line 532-541: Update getConnectedPointIndices to validate both raw link
endpoints with isPointIndex before adding either endpoint to its result; invalid
links such as [0, 3] must contribute neither endpoint, while valid links retain
their existing behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 10db26fb-ac6b-4947-b4e0-e77bcabb5546
📒 Files selected for processing (3)
src/index.tssrc/modules/GraphData/index.tssrc/modules/Lines/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index.ts
Closed
The endpoint validation added with the link fixes covered the adjacency lists and the render path, but `getConnectedPointIndices` reads `links` directly and so bypassed both — asked about a link whose target is not a real point, it returned that index anyway. With 3 points and a link [0, 3] it answered [0, 3], handing the caller a point that does not exist; a multi-link query mixed the phantom in with valid endpoints. - Both endpoints are checked with `isPointIndex`, and a link with either one invalid contributes neither. That matches what the adjacency build already skips, so the two readers now agree. - The link index itself must be an integer. `0.5` passed the range test and then read `links[1]` and `links[2]` — one endpoint from each of two neighbouring links — reporting a pair that was never a link. The other index getters already reject a fractional index. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Stukova Olya <stukova.o@gmail.com>
trackPointPositionsByIndices baked each tracked index into its texel of the position grid once, at call time. The grid width is ceil(sqrt(count)), so any later point-count change relaid the grid out while the table kept addressing the old layout: a tracked label silently followed whichever point now occupied the stale texel, and an index with no point behind it read out of range and reported (0, 0) — a coordinate that looks legitimate. No stage failed, so nothing surfaced. The table now stores the raw point index — the question, not the answer — and the shader derives the texel at read time from the width the positions texture has right now (textureSize(), valid here because the shader samples that texture). Staleness is not fixed but inexpressible: there is no baked mapping left to rot, and no new state tracking it. - precision highp int is declared: raw indices exceed mediump int's 16-bit spec minimum, and fragment shaders default to mediump. Integer texel math is required — a GPU probe showed float mod(33.0, 33.0) returning 33 (the boundary-floor failure the texelFetch rework documented), while the int path was exact for every width at indices up to 2^24 - 1. - float32 carries an index exactly to 2^24 — the ceiling every float-carried index in the engine already lives under. - The tracked set becomes declarative: an index follows its point whenever the point exists. The map omits an index at or past the current count (the absent-point contract) and the entry returns if the count grows back; the array keeps the slot as NaN to stay aligned. Both readbacks guard with isPointIndex. - The bake no longer needs the grid width, so tracking can be set up before the first setPointPositions call. Verified in Storybook against real Graph instances, points laid out exactly as the internal grid stores them: growing 9 -> 16 relayouts the grid and the tracked entry follows point 5's new position instead of reporting point 6 at the stale texel; re-tracking is a no-op; shrinking below the index yields no entry and growing back restores it; a NaN-removed point stays omitted; tracking [99] on 9 points yields an empty map. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Stukova Olya <stukova.o@gmail.com>
The wrap-around vertex used float mod — a WebGL 1 leftover. GPU division is reciprocal-based, so mod(N, N) can return N (33 fails on ANGLE Metal; 32 and 100 are exact): the last edge then closed onto a zero-filled padding texel at (0, 0), and a 33-vertex lasso selected four outsiders while dropping the point it enclosed. Integer % cannot round. A 32-vertex control selects exactly the enclosed point before and after. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Fragment shaders default int to mediump — a 16-bit spec minimum, and SwiftShader reports exactly 16. The near-field pass decomposes a raw point index with % and /, so past 32 767 points the fetch could land on the wrong texel. Same declaration as track-positions.frag; verified behavior-neutral on ANGLE Metal (repulsion identical before and after). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Stukova Olya <stukova.o@gmail.com>
…ndings The data-texture-addressing entry recorded the tracked-index re-bake as owed; it is now resolved by the entry's own rule — the table stores raw indices and the shader derives texels at read time. Also recorded: the WebGL 1 origin of the half-texel idiom (GLSL ES 1.00 reserved %), the lasso closing-edge fix as the codebase's live instance of the measured mod(N, N) failure, the highp int declarations in the two raw-index fragment shaders with SwiftShader's 16-bit mediump report as evidence, and the deliberate decisions to leave bounded ints and sampler precision at their defaults. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Stukova Olya <stukova.o@gmail.com>
The public docs promised contracts the code never kept. render() claimed it "does NOT modify simulation state" — but the simulationAlpha argument sets the alpha, and starting a position transition pauses a running simulation (behavior added with the GPU-transition work; the claim predates it). create() claimed to apply pending data changes "without calling render()" — but it never ingests input arrays (graph.update() runs only in the render path), so set* data cannot take effect through it; that wording came from a docs sweep and described intent, not behavior. start() claimed to control "only the simulation state, not rendering" while requesting frames and force-ending an active position transition; unpause() force-ends transitions too and said nothing. - render(): state the real contract — no simulation start/stop, with the two exceptions named by argument (simulationAlpha, transitionDuration). - create(): describe it as the flag-gated GPU upload stage of the render pipeline, and point at render(undefined) / render(undefined, 0) for applying new data; drop the stale "public contract" inline comment. - start()/unpause(): name the position-transition interruption (onTransitionEnd fires with interrupted: true), matching what the configuration docs already document under onTransitionEnd. - Mirror all four entries in the Storybook API reference. Docs now promise exactly what the code does. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/stories/api-reference.mdx (1)
704-704: 📐 Maintainability & Code Quality | 🔵 TrivialResolve validation failures before opening the pull request.
npm run lintfails with four formatting errors.npm run buildfails becausevite-plugin-dtsis missing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/stories/api-reference.mdx` at line 704, Resolve the validation failures before submitting the pull request: format the changed documentation text in the api-reference content to satisfy npm run lint, and restore or install the missing vite-plugin-dts dependency required by npm run build. Verify both commands pass.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/stories/api-reference.mdx`:
- Line 704: Resolve the validation failures before submitting the pull request:
format the changed documentation text in the api-reference content to satisfy
npm run lint, and restore or install the missing vite-plugin-dts dependency
required by npm run build. Verify both commands pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7cb60463-7f40-4b31-9759-979e165e0faf
📒 Files selected for processing (2)
src/index.tssrc/stories/api-reference.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Independent bug fixes — no breaking changes. Each section: what was broken, what changed. Mechanism details live in the commit messages.
Multiple Graph instances no longer clobber each other
The library wrote to shared globals, so a second Graph on the page broke the first:
.cosmosnamespace ondocumentis now per-instancedocument.documentElement— now on the graph's containerdestroy()Runtime config changes now take effect immediately
Three config properties accepted new values via
setConfigwithout any effect:pointSamplingDistance/linkSamplingDistance— the sampling grids were only rebuilt on canvas resize; now rebuilt on changepointDefaultSize— point image sizes default to a copy of point sizes but were never re-derived; now refreshed (explicit user-set image sizes preserved)enableZoom: false— onlywheel.zoomwas detached; now blocks wheel, double-click, pinch and double-tap, while panning and programmatic zoom keep workingRead data textures by index, not by coordinate
A shader reading a per-element texture knows exactly which element it wants, but reads used
texture()atindex / textureSize— a texel boundary, where the sampler's floor can land on the previous element (enough to pull a whole cluster onto its neighbour). Every data-texture read is nowtexelFetch(): integer texel in, that texel out.texture()remains only for genuine UV sampling — the image atlas.Invalid input arrays no longer corrupt the layout
NaNor negative entry insetLinkStrengthpinned both endpoints of that link at(0, 0)while the rest of the graph laid out normally — invalid values now resolve to the degree-based defaultsetPointPositionsarray made the point count fractional and threwRangeError: Invalid array length— the trailing value is now dropped with a warningRect / polygon search no longer returns points that don't exist
A 5-point graph could answer a select-all with 9 indices. Positions are stored in a square grid —
ceil(√5) = 3, so 9 slots for 5 points:(0, 0)is a real place — the corner of the space. The search checks every slot, so once the view is zoomed out far enough that the corner is on screen, a selection over it picks up the ghosts:Perfect-square counts (4, 9, 16, …) have no spare slots and never failed, which is why this looked intermittent.
Collision covers its full interaction range
Colliding pairs could sit in grid cells the 3×3 scan never compares: a cell measured one effective radius, but touching points interact at two. A cell-size floor hid this at the default point size; anything larger overlapped freely — 200 points at size 30 settled with 79 interpenetrating pairs. Two more parts of the same fix: a cell's own-point contribution biased measured distances short, and the grid fitting (round-up plus a 32-cell minimum) undid the corrected cell size above point size 128. The same scenarios now settle with zero overlapping pairs; small points are unchanged.
Unblended links draw opaque edges instead of a bright fringe
With
linkBlending: false, antialiasing fringes wrote full RGB at partial alpha, so link edges rendered brighter than the solid core. The fragment shader now separates geometric coverage from color alpha and writes opaque pixels when blending is off — without erasing thin links, which a naive hard cut would have. Blended and picking paths are unchanged.Link endpoints must be real points
An odd-length links array silently killed the graph — blank canvas, every getter throwing, nothing in the console. Invalid endpoints (out-of-range, negative, fractional,
NaN) flowed through untouched:getNeighboringPointIndices/getConnectedPointIndicesreturned points that don't exist, and a bogus link was drawn.The odd trailing value is now dropped (via a view — the caller's array is untouched), and endpoints are validated inside the loops that already walk the links, so no pass is added. Invalid links are neutralised in place rather than removed: link indices are public API (
onLinkClick,focusedLinkIndex, …) and compacting would renumber them.pair()shared the odd-length crash and is fixed too.Tracked points follow the point, not a stale texel
trackPointPositionsByIndicesbaked each tracked index into its texel of the position grid once, at call time — but the grid relayouts whenever the point count changes (width isceil(√count)), so a later grow or shrink left the table addressing the old layout: the tracked entry silently reported whichever point now occupied the stale texel, and an index with no point behind it read(0, 0). The table now stores the raw index, and the shader derives the texel at read time from the texture's live width — no baked mapping left to go stale. The tracked set becomes declarative: an index follows its point whenever it exists, reports nothing while the count excludes it, and resumes if the count grows back. Fragment-stage index math also gainsprecision highp int: a GPU probe showed floatmod(33.0, 33.0)returning33, and mediump int — the fragment-stage default — is only 16-bit by spec.Found but not fixed here
Surfaced during review, out of scope for this PR:
render()revalidates everything on every call — all twelve data channels re-checked and the adjacency lists rebuilt whether anything changed or not: 32–40 ms per call at 100k points / 200k links, against a ~0.2 ms floor. Only affects apps that callrender()repeatedly (streaming / animating data); the internal frame loop bypasses it. Worth fixing only if that's a use case we want to support.create()doesn't apply data changes — and loses them — it's documented as "apply changes without callingrender()", but it uploads the previously validated data and clears the pending flags, so even a followingrender()can't recover the change:setPointColors(red)→create()→render()leaves the canvas white whilegetPointColors()reports red. The fix is to makecreate()validate → upload → bind.Suggested follow-up: a test system
Every fix above was verified with throwaway headless-Chromium harnesses driving real
Graphinstances — effective, but ad-hoc and outside the repo. The natural in-repo shape is Vitest browser mode (Playwright provider): tests are plain TypeScript that importGraphthrough the existing Vite config and aliases, and run against real WebGL2 in headless Chromium —npm run test, with watch mode for development. About ten of this PR's harnesses migrate directly (link validation, collision settling, search ghosts, tracking semantics, multi-instance isolation); gesture-driven checks (double-tap, pan, hover) need Playwright's input escape hatch and can follow as a second phase. CI would run SwiftShader — guarding regressions on one GPU stack, not multi-vendor correctness. Repro stories stay local; tests become the durable artifact.Summary by CodeRabbit
Bug Fixes
Improvements
Documentation