Cosmos 2.0 #128
Cosmos 2.0 #128
Conversation
WalkthroughThe pull request introduces a significant update to the Cosmos graph visualization library, transitioning from version 1.6.0 to 2.0.0. The changes are comprehensive, focusing on modernizing the codebase, enhancing terminology from "nodes" to "points," and improving the overall architecture. Key modifications include simplifying generic type parameters, updating shader and configuration files, introducing Storybook for documentation, and refactoring data handling methods. The project now supports more flexible point and link management, clustering features, and provides a more streamlined development experience. Changes
Sequence DiagramsequenceDiagram
participant User
participant Graph
participant GraphData
participant Simulation
participant Renderer
User->>Graph: Create graph
Graph->>GraphData: Set point positions
Graph->>GraphData: Set links
Graph->>Simulation: Configure parameters
Graph->>Renderer: Render graph
User->>Graph: Interact (zoom, drag)
Simulation->>Graph: Update point positions
Renderer->>Graph: Redraw
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🔭 Outside diff range comments (1)
src/modules/ForceManyBody/force-level.frag (1)
Line range hint
23-41: Consider optimizing velocity calculation.The
calculateAdditionalVelocityfunction performs multiple trigonometric operations. Consider using vector operations directly:vec2 calculateAdditionalVelocity (vec2 ij, vec2 pp) { vec2 add = vec2(0.0); vec4 centermass = texture2D(levelFbo, ij); if (centermass.r > 0.0 && centermass.g > 0.0 && centermass.b > 0.0) { vec2 centermassPosition = vec2(centermass.rg / centermass.b); vec2 distVector = pp - centermassPosition; float l = dot(distVector, distVector); float dist = sqrt(l); if (l > 0.0) { - float angle = atan(distVector.y, distVector.x); float c = alpha * repulsion * centermass.b; float distanceMin2 = 1.0; if (l < distanceMin2) l = sqrt(distanceMin2 * l); float addV = c / sqrt(l); - add = addV * vec2(cos(angle), sin(angle)); + add = addV * normalize(distVector); } } return add; }
🧹 Nitpick comments (73)
src/modules/Lines/index.ts (1)
109-112: Optimize buffer initialization in thedrawmethodIn the
drawmethod, buffers such ascolorBuffer,widthBuffer, andarrowBufferare initialized during the draw call if they are not already initialized. To improve rendering performance, consider initializing these buffers beforehand to avoid any potential overhead during the rendering loop.src/modules/ForceManyBody/index.ts (1)
48-52: Optimize random values generationThe random values generation can be optimized by using vectorized operations or ensuring that the random numbers are within the desired range without unnecessary computation.
While this is not critical, consider using more efficient methods for generating random values if performance is a concern.
src/modules/GraphData/index.ts (3)
4-28: Consider encapsulating class properties to enhance encapsulationThe
GraphDataclass declares numerous public properties (lines 4-28), which may expose internal state unnecessarily and risk unintended external modifications. To improve encapsulation and maintainability, consider making these properties private or protected and providing public getter and setter methods as needed.
63-88: Refactor repetitive update methods to reduce code duplicationThe methods
updatePointColor(lines 63-88) andupdateLinkColor(lines 119-145) contain similar logic for handling default colors and validating input arrays. Extracting the common logic into shared utility functions can minimize code duplication and enhance maintainability.Also applies to: 119-145
93-110: Refactor size update methods to eliminate duplicate codeThe methods
updatePointSize(lines 93-110) andupdateLinkWidth(lines 150-166) share similar patterns for updating sizes with default values and input validation. Consider refactoring to consolidate common code into reusable functions, improving readability and reducing duplication.Also applies to: 150-166
src/config.ts (4)
18-18: Remove informal notation '👇' in documentation commentThe documentation comment at line 18 includes an informal notation '👇.'. To maintain a professional tone, consider removing this symbol.
95-95: Correct extra space in documentation commentAt line 95, there is an extra space between sentences: "index. A ring will be highlighted...". Please remove the extra space for consistency.
Line range hint
400-400: Fix grammatical error in documentationIn the comment at line 400, it says "Points 4 is unclustered." It should be "Point 4 is unclustered."
17-20: Address the TODO regardingdisableSimulationparameterThere is a TODO comment at lines 17-20 suggesting a reconsideration of the
disableSimulationparameter's logic. Reviewing and clarifying this parameter's behavior will enhance the configurability and usability of the simulation.Would you like assistance in rethinking the logic or updating the implementation for
disableSimulation?src/modules/Points/index.ts (3)
2-3: Remove unused import statementsThe import statements at lines 2-3 are commented out. If these imports are no longer needed, consider removing them to keep the codebase clean.
Line range hint
491-507: Refactor duplicate logic in drawing highlighted pointsThe
drawmethod (lines 491-507) contains similar code blocks for drawing rings around hovered and focused points. Refactoring this into a shared helper function can reduce duplication and improve code maintainability.
608-637: Remove unused commented code to clean up the codebaseThe method
rescaleInitialNodePositions(lines 608-637) is commented out. If this code is obsolete or unnecessary, consider deleting it to declutter the codebase.src/modules/Points/fill-sampled-points.frag (1)
1-9: LGTM! Consider performance optimization for mobile.The shader implementation is clean and straightforward. For mobile devices, you might consider using
mediumpprecision if high precision isn't strictly necessary, as it could offer better performance on some devices.#ifdef GL_ES -precision highp float; +precision mediump float; #endifsrc/stories/experiments/full-mesh.ts (2)
5-5: Consider extracting magic numbers into named constants.The parameters
40, 30, 15, 1.0would be more maintainable and self-documenting if extracted into named constants.+const MESH_CONFIG = { + POINT_COUNT: 40, + LINK_COUNT: 30, + RADIUS: 15, + DENSITY: 1.0, +} as const + export const FullMeshStory = (): HTMLDivElement => { - const { pointPositions, links, pointColors } = generateMeshData(40, 30, 15, 1.0) + const { pointPositions, links, pointColors } = generateMeshData( + MESH_CONFIG.POINT_COUNT, + MESH_CONFIG.LINK_COUNT, + MESH_CONFIG.RADIUS, + MESH_CONFIG.DENSITY + )
5-10: Add type safety for the destructured object.Consider adding TypeScript types for better type safety and documentation.
+interface MeshData { + pointPositions: number[][] + links: number[][] + pointColors: number[][] +} export const FullMeshStory = (): HTMLDivElement => { - const { pointPositions, links, pointColors } = generateMeshData(40, 30, 15, 1.0) + const { pointPositions, links, pointColors }: MeshData = generateMeshData(40, 30, 15, 1.0)src/modules/Points/draw-highlighted.frag (1)
12-15: Consider adding a comment explaining the circular shape calculation.The shader correctly implements circular point highlighting with anti-aliasing using dot product, but the mathematical reasoning could be documented for maintainability.
void main () { + // Calculate squared distance from center (0,0) for circular shape float r = dot(vertexPosition, vertexPosition); + // Apply smooth transition at edges for anti-aliasing float opacity = smoothstep(r, r * smoothing, 1.0); float stroke = smoothstep(width, width * smoothing, r); gl_FragColor = vec4(color.rgb, opacity * stroke * color.a * pointOpacity); }src/modules/Points/drag-point.frag (1)
5-7: Consider documenting the texture format.The shader relies on specific texture formats (particularly the blue channel for index storage). This should be documented for maintainability.
+// positionsTexture format: (x, y, index, unused) uniform sampler2D positionsTexture; uniform vec2 mousePos; +// index: -1.0 means no point is being dragged uniform float index;src/modules/Points/track-positions.frag (1)
5-7: Document texture size requirements.The shader uses
pointsTextureSizefor texture coordinate calculation. It's important to document any size constraints or requirements.uniform sampler2D positionsTexture; uniform sampler2D trackedIndices; +// pointsTextureSize: Must be a power of 2 for proper texture sampling uniform float pointsTextureSize;src/stories/clusters/worm.ts (2)
6-6: Document magic numbers in generateMeshData callThe parameters (100, 100, 1000, 1.0) lack documentation explaining their purpose and impact. Consider extracting these as named constants with clear documentation.
+// Define mesh parameters with clear documentation +const MESH_POINTS = 100 +const MESH_CLUSTERS = 100 +const MESH_SIZE = 1000 +const MESH_DENSITY = 1.0 + const { pointPositions, pointColors, links, linkColors, pointClusters } = generateMeshData( - 100, 100, 1000, 1.0 + MESH_POINTS, + MESH_CLUSTERS, + MESH_SIZE, + MESH_DENSITY )
8-10: Document simulation parameters and their effectsThe simulation parameters (gravity, repulsion, linkSpring) would benefit from documentation explaining their effects on the visualization.
+// Simulation parameters: +// - gravity: Controls the overall attraction force (0.5 = moderate attraction) +// - repulsion: Controls how strongly points repel each other (1 = standard repulsion) +// - linkSpring: Controls the spring force of connections (1 = standard tension) const { div } = createCosmos({ simulationGravity: 0.5, simulationRepulsion: 1, simulationLinkSpring: 1,src/modules/ForceManyBody/calculate-level.vert (1)
Line range hint
18-22: Document the mathematical calculationsConsider adding comments explaining the cell position calculations and the transformation to clip space coordinates.
+ // Calculate cell indices based on point position and cell size float n = floor(pointPosition.x / cellSize); float m = floor(pointPosition.y / cellSize); + // Transform cell position to clip space coordinates (-1 to 1) vec2 levelPosition = 2.0 * (vec2(n, m) + 0.5) / levelTextureSize - 1.0;.storybook/preview.ts (2)
17-23: Consider documenting theme color choicesThe background colors are hardcoded without explanation. Consider adding comments explaining the choice of these specific colors, especially
#192132for dark theme.backgrounds: { default: 'dark', values: [ + // Default light theme background { name: 'light', value: '#fff' }, + // Custom dark theme matching Cosmos design system { name: 'dark', value: '#192132' }, ], },
4-25: Enhance type safety for theme configurationConsider creating a type-safe theme configuration object to prevent potential misconfigurations.
type ThemeConfig = { name: 'light' | 'dark'; value: string; }; const themeValues: ThemeConfig[] = [ { name: 'light', value: '#fff' }, { name: 'dark', value: '#192132' }, ]; const preview: Preview = { parameters: { // ... other config backgrounds: { default: 'dark' as const, values: themeValues, }, }, };src/stories/clusters/radial.ts (1)
6-10: Define types for mesh data structureConsider adding TypeScript interfaces for the mesh data structure to improve type safety and documentation.
interface MeshData { pointPositions: Float32Array; pointColors: Float32Array; pointSizes: Float32Array; links: Float32Array; linkColors: Float32Array; linkWidths: Float32Array; pointClusters: Float32Array; clusterPositions: Float32Array; clusterStrength: number; }src/modules/ForceMouse/force-mouse.frag (1)
Line range hint
16-19: Consider optimizing distance calculationThe current distance calculation could be optimized by avoiding the square root operation when possible.
vec2 distVector = mouse - pointPosition.rg; - float dist = sqrt(dot(distVector, distVector)); - dist = max(dist, 10.0); + float distSquared = dot(distVector, distVector); + distSquared = max(distSquared, 100.0); // 10.0 * 10.0 + float dist = sqrt(distSquared);src/modules/ForceGravity/force-gravity.frag (1)
17-23: Consider caching centerPosition calculationThe
centerPositioncalculation could be moved to a uniform variable to avoid recalculation for each fragment.+ uniform vec2 centerPosition; // Add to uniforms at the top void main() { vec4 pointPosition = texture2D(positionsTexture, textureCoords); vec4 velocity = vec4(0.0); - vec2 centerPosition = vec2(spaceSize / 2.0); vec2 distVector = centerPosition - pointPosition.rg;src/stories/clusters/with-labels.ts (2)
7-7: Consider extracting magic numbers into named constantsThe parameters passed to
generateMeshData(100, 100, 15, 1.0) would be clearer as named constants with meaningful descriptions.+ const MESH_WIDTH = 100 + const MESH_HEIGHT = 100 + const CLUSTER_COUNT = 15 + const NOISE_FACTOR = 1.0 - const { pointPositions, pointColors, pointClusters } = generateMeshData(100, 100, 15, 1.0) + const { pointPositions, pointColors, pointClusters } = generateMeshData( + MESH_WIDTH, + MESH_HEIGHT, + CLUSTER_COUNT, + NOISE_FACTOR + )
6-24: Add cleanup handling for event listenersThe story should return a cleanup function to remove event listeners when the component is unmounted.
- export const WithLabelsStory = (): HTMLDivElement => { + export const WithLabelsStory = (): { element: HTMLDivElement; cleanup: () => void } => { // ... existing code ... - return div + return { + element: div, + cleanup: () => { + graph.setConfig({ + onZoom: null, + onSimulationTick: null, + }) + } + } }src/modules/Clusters/calculate-centermass.vert (2)
15-16: Consider precision loss in texture coordinate calculationWhen dividing
pointIndicesbypointsTextureSize, consider using a higher precision type or adding a small epsilon to prevent floating-point precision issues.- vec4 pointPosition = texture2D(positionsTexture, pointIndices / pointsTextureSize); + vec4 pointPosition = texture2D(positionsTexture, (pointIndices + 0.5) / pointsTextureSize);
24-25: Consider configurable point sizeThe hardcoded
gl_PointSize = 1.0might not be suitable for all use cases. Consider making it configurable via a uniform.+uniform float pointSize; gl_Position = vec4(xy, 0.0, 1.0); -gl_PointSize = 1.0; +gl_PointSize = pointSize;src/modules/Points/draw-points.frag (1)
11-12: Consider making smoothing configurable.While
smoothingas a constant works, consider making it a uniform variable to allow runtime adjustment of point edge smoothness.-const float smoothing = 0.9; +uniform float smoothing;src/modules/Points/fill-sampled-points.vert (2)
16-19: Consider documenting the texture sampling logic.While the code is functional, adding comments explaining the texture sampling and space transformation would improve maintainability.
+ // Sample point position from texture using normalized coordinates vec4 pointPosition = texture2D(positionsTexture, (pointIndices + 0.5) / pointsTextureSize); + // Convert position to normalized device coordinates vec2 p = 2.0 * pointPosition.rg / spaceSize - 1.0; + // Adjust for aspect ratio p *= spaceSize / screenSize; + // Apply transformation matrix vec3 final = transformationMatrix * vec3(p, 1);
22-22: Consider using more descriptive variable names.The variable
indexcould be renamed to better reflect its purpose (e.g.,pointIndexorlinearIndex).- float index = pointIndices.g * pointsTextureSize + pointIndices.r; + float pointIndex = pointIndices.g * pointsTextureSize + pointIndices.r;src/modules/Shared/buffer.ts (1)
Line range hint
11-20: Good refactoring to improve separation of concerns!The function has been well refactored to focus solely on index computation, making it:
- More reusable (can be used with different buffer types)
- Easier to test (pure function)
- More flexible (consumers control buffer creation)
Consider adding JSDoc comments to document:
- The relationship between textureSize and the returned array size
- The format of the generated indexes (x,y pairs)
src/modules/ForceMouse/index.ts (1)
12-26: Consider moving initialization to constructorWhile the null check is good, the current pattern delays program initialization. Consider:
- Moving initialization to constructor
- Making runCommand readonly after initialization
Here's a suggested refactor:
- private runCommand: regl.DrawCommand | undefined + private readonly runCommand: regl.DrawCommand public constructor() { + super() + const { reglInstance, config, store, points } = this + this.runCommand = reglInstance({ + frag: forceFrag, + vert: updateVert, + framebuffer: () => points?.velocityFbo as regl.Framebuffer2D, + primitive: 'triangle strip', + count: 4, + attributes: { vertexCoord: createQuadBuffer(reglInstance) }, + uniforms: { + positionsTexture: () => points?.previousPositionFbo, + mousePos: () => store.mousePosition, + repulsion: () => config.simulationRepulsionFromMouse, + }, + }) } - public initPrograms(): void { - // ... current initialization code - }src/modules/Drag/index.ts (2)
5-8: Add TypeScript types for event handlersConsider adding proper types for the drag event handlers to improve type safety.
import { D3DragEvent } from 'd3-drag' interface DragHandlers { onDragStart?: (event: D3DragEvent<HTMLCanvasElement, undefined, undefined>) => void onDrag?: (event: D3DragEvent<HTMLCanvasElement, undefined, undefined>) => void onDragEnd?: (event: D3DragEvent<HTMLCanvasElement, undefined, undefined>) => void }
29-32: Add validation for required config propertiesThe constructor should validate that required config properties (drag event handlers) are properly typed.
public constructor(store: Store, config: GraphConfigInterface) { + if (!store) throw new Error('Store is required') + if (!config) throw new Error('Config is required') this.store = store this.config = config }CONTRIBUTING.md (1)
9-11: Enhance development setup instructionsThe development process section could benefit from more detailed setup instructions, including:
- Prerequisites (Node.js version, etc.)
- Installation steps
- Available npm scripts
Would you like me to propose expanded documentation for the development setup?
src/stories/experiments.stories.ts (3)
12-15: Enhance story metadataConsider adding more metadata to improve story documentation:
componentfield for proper categorizationparameters.docs.descriptionfor component descriptionconst meta: Meta<CosmosStoryProps> = { title: 'Experiments', + parameters: { + docs: { + description: 'Experimental visualizations showcasing mesh generation capabilities.', + }, + }, }
19-22: Consider moving source code parameters to a shared configurationThe
sourceCodeAddonParamscould be moved to a shared configuration file since it might be reused across different story files.Consider creating a
src/stories/shared/source-code-params.tsfile to maintain these parameters centrally.
39-40: Remove ESLint disable commentInstead of disabling the ESLint rule, consider configuring it properly in the ESLint configuration file.
-// eslint-disable-next-line import/no-default-export export default metaAdd to
.eslintrc:{ "overrides": [ { "files": ["*.stories.ts"], "rules": { "import/no-default-export": "off" } } ] }src/modules/Points/find-points-on-area-selection.frag (1)
29-32: Consider optimizing multiple pointSizeF callsThe shader makes multiple calls to
pointSizeF(size)for calculating bounds. Consider storing the result in a variable to avoid redundant calculations.vec4 pSize = texture2D(pointSize, textureCoords); float size = pSize.r * sizeScale; + float scaledSize = pointSizeF(size); - float left = 2.0 * (selection[0].x - 0.5 * pointSizeF(size)) / screenSize.x - 1.0; - float right = 2.0 * (selection[1].x + 0.5 * pointSizeF(size)) / screenSize.x - 1.0; - float top = 2.0 * (selection[0].y - 0.5 * pointSizeF(size)) / screenSize.y - 1.0; - float bottom = 2.0 * (selection[1].y + 0.5 * pointSizeF(size)) / screenSize.y - 1.0; + float left = 2.0 * (selection[0].x - 0.5 * scaledSize) / screenSize.x - 1.0; + float right = 2.0 * (selection[1].x + 0.5 * scaledSize) / screenSize.x - 1.0; + float top = 2.0 * (selection[0].y - 0.5 * scaledSize) / screenSize.y - 1.0; + float bottom = 2.0 * (selection[1].y + 0.5 * scaledSize) / screenSize.y - 1.0;Also applies to: 34-40
src/stories/clusters.stories.ts (2)
7-11: Consider using dynamic imports for raw source codeLoading all raw source code upfront might impact initial load time. Consider using dynamic imports if the source code viewing is not immediately needed.
-import createCosmosRaw from './create-cosmos?raw' -import generateMeshDataRaw from './generate-mesh-data?raw' -import withLabelsStory from './clusters/with-labels?raw' -import wormStory from './clusters/worm?raw' -import radialStory from './clusters/radial?raw' +const loadSourceCode = async (story: string) => { + switch (story) { + case 'worm': + return import('./clusters/worm?raw') + case 'radial': + return import('./clusters/radial?raw') + case 'withLabels': + return import('./clusters/with-labels?raw') + default: + return '' + } +}
24-27: Consider centralizing source code parametersThe
sourceCodeAddonParamsarray is repeated across stories. Consider creating a helper function to generate story parameters.+const createStoryParameters = (storyCode: string) => ({ + sourceCode: [ + { name: 'Story', code: storyCode }, + ...sourceCodeAddonParams, + ], +}) export const Worm = WormStory as Story -Worm.parameters = { - sourceCode: [ - { name: 'Story', code: wormStory }, - ...sourceCodeAddonParams, - ], -} +Worm.parameters = createStoryParameters(wormStory)Also applies to: 31-34, 39-42, 47-50
src/variables.ts (1)
23-27: Document new configuration optionsSeveral new features have been added:
- Point interaction controls (hover, focus)
- Drag functionality
- View fitting controls
- Point sampling
Consider adding JSDoc comments to document these new options and their impact.
Also applies to: 47-52
src/modules/ForceManyBody/force-centermass.frag (1)
41-48: Consider optimizing texture readsMultiple texture reads (
positionsTexture,randomValues) could be cached if used multiple times.src/modules/Points/draw-points.vert (1)
42-46: Consider optimizing matrix multiplicationThe position transformation could potentially be optimized by combining multiple matrix operations.
- vec2 normalizedPosition = 2.0 * point / spaceSize - 1.0; - normalizedPosition *= spaceSize / screenSize; - vec3 finalPosition = transformationMatrix * vec3(normalizedPosition, 1); + vec2 normalizedPosition = 2.0 * point / screenSize - spaceSize / screenSize; + vec3 finalPosition = transformationMatrix * vec3(normalizedPosition, 1);src/helper.ts (1)
33-35: Consider adding input validation for RGB values.The luminance calculation is correct, but the function could benefit from input validation to ensure RGB values are within the valid range [0-255].
export function rgbToBrightness (r: number, g: number, b: number): number { + // Ensure RGB values are within valid range + const clampColor = (c: number) => clamp(c, 0, 255) + r = clampColor(r) + g = clampColor(g) + b = clampColor(b) return 0.2126 * r + 0.7152 * g + 0.0722 * b }src/modules/Points/find-hovered-point.vert (2)
39-42: Consider adding a comment explaining the normalization constants.The transformation from texture space to normalized device coordinates could benefit from documentation explaining the mathematical basis of the scaling factors.
// Transform point position to normalized device coordinates + // Scale from [0,spaceSize] to [-1,1] range, then adjust for aspect ratio vec2 normalizedPoint = 2.0 * pointPosition.rg / spaceSize - 1.0; normalizedPoint *= spaceSize / screenSize;
49-53: Consider optimizing the point-mouse intersection test.The current implementation performs the distance calculation for all points. Consider adding an early exit for points far from the mouse position using a coarser bounding box test.
// Check if the mouse is within the point radius + // Quick bounding box test before expensive distance calculation + vec2 delta = abs(pointScreenPosition - mousePosition); + if (delta.x > pointRadius / ratio || delta.y > pointRadius / ratio) return; if (euclideanDistance(pointScreenPosition.x, mousePosition.x, pointScreenPosition.y, mousePosition.y) < pointRadius / ratio) {src/stories/create-cluster-labels.ts (1)
17-28: Consider extracting font stack to a constant.The font family stack could be moved to a shared constant for consistency across the application.
+const SYSTEM_FONT_STACK = [ + '"Nunito Sans"', + '-apple-system', + '".SFNSText-Regular"', + '"San Francisco"', + 'BlinkMacSystemFont', + '"Segoe UI"', + '"Helvetica Neue"', + 'Helvetica', + 'Arial', + 'sans-serif', +].join(', ') - contentLabel.style.fontFamily = [ - '"Nunito Sans"', - '-apple-system', - '".SFNSText-Regular"', - '"San Francisco"', - 'BlinkMacSystemFont', - '"Segoe UI"', - '"Helvetica Neue"', - 'Helvetica', - 'Arial', - 'sans-serif', - ].join(', ') + contentLabel.style.fontFamily = SYSTEM_FONT_STACKsrc/modules/Points/draw-highlighted.vert (1)
22-30: Consider optimizing matrix transformation calculationsWhile the function rename to
calculatePointSizeimproves clarity, the repeated use oftransformationMatrix[0][0]could be optimized by storing it in a local variable.float calculatePointSize(float size) { float pSize; + float scale = transformationMatrix[0][0]; if (scalePointsOnZoom) { - pSize = size * transformationMatrix[0][0]; + pSize = size * scale; } else { - pSize = size * min(5.0, max(1.0, transformationMatrix[0][0] * 0.01)); + pSize = size * min(5.0, max(1.0, scale * 0.01)); } return min(pSize, maxPointSize); }src/stories/create-cosmos.ts (3)
3-16: Consider documenting property types and unitsThe
CosmosStoryPropsinterface would benefit from JSDoc comments explaining:
- Expected ranges for array values
- Units for sizes and positions
- Purpose of the commented out
linkStrengthproperty
25-47: Document magic numbers in default configurationThe default configuration contains several magic numbers that would benefit from documentation or constants:
pointSize: 3simulationLinkSpring: 2simulationRepulsion: 0.5simulationGravity: 0.02simulationFriction: 0.7+ // Configuration constants with meaningful names + const DEFAULT_POINT_SIZE = 3; // pixels + const DEFAULT_LINK_SPRING = 2; // force multiplier + const DEFAULT_REPULSION = 0.5; // force multiplier + const DEFAULT_GRAVITY = 0.02; // force multiplier + const DEFAULT_FRICTION = 0.7; // velocity dampening const config: GraphConfigInterface = { backgroundColor: '#212C42', - pointSize: 3, + pointSize: DEFAULT_POINT_SIZE, // ... other properties - simulationLinkSpring: 2, - simulationRepulsion: 0.5, - simulationGravity: 0.02, - simulationFriction: 0.7, + simulationLinkSpring: DEFAULT_LINK_SPRING, + simulationRepulsion: DEFAULT_REPULSION, + simulationGravity: DEFAULT_GRAVITY, + simulationFriction: DEFAULT_FRICTION,
64-65: Consider making initial zoom configurableThe hardcoded zoom value of 0.9 could be made configurable through props.
+ type CosmosStoryProps = GraphConfigInterface & { + initialZoom?: number; + // ... other properties + } // ... in createCosmos function - graph.zoom(0.9) + graph.zoom(props.initialZoom ?? 0.9).eslintrc (1)
25-35: LGTM: Enhanced import ordering configurationThe updated import ordering rules provide better organization by:
- Including an "internal" group
- Properly handling @/** imports
Consider adding newlines between import groups for better readability:
"import/order": ["error", { "groups": ["builtin", "external", "internal", "parent", "sibling", "index", "object"], + "newlines-between": "always", "pathGroups": [ { "pattern": "@/**", "group": "internal" } ], } ],src/modules/ForceLink/force-spring.ts (1)
41-43: Consider optimizing texture readsThe shader performs multiple texture reads within the loop. Consider:
- Caching texture reads outside the loop where possible
- Using texture arrays if supported by the target platform
Also applies to: 51-51, 56-56
src/modules/ForceManyBody/quadtree-frag-shader.ts (1)
69-70: Consider caching texture readsThe texture reads for
pointPositionandrandomare used multiple times in the quadtree traversal. Consider caching these values in local variables to potentially improve performance.package.json (1)
13-13: New build system and development toolsAdded new scripts:
build:vite: Modern build systemstorybookandbuild:storybook: Component development environmentThis improves the development experience but requires documentation updates.
Consider:
- Adding README section about Storybook usage
- Documenting the build process changes
- Adding examples in Storybook
Also applies to: 16-18
src/modules/ForceManyBodyQuadtree/quadtree-frag-shader.ts (1)
7-13: Consider adding parameter documentation.While the variable names are clear, adding JSDoc-style comments for the parameters would enhance maintainability.
+/** + * @param {number} startLevel - The initial level in the quadtree hierarchy + * @param {number} maxLevels - The maximum number of levels in the quadtree + * @returns {string} The generated GLSL shader code + */ export function forceFrag(startLevel: number, maxLevels: number): string {cosmos-2-0-migration-notes.md (2)
9-13: Add performance benefits section.While the guide explains the technical changes, it would be helpful to quantify the performance improvements from using WebGL-compatible data structures.
Consider adding a section like:
+#### Performance Benefits + +The transition to WebGL-compatible data structures brings significant performance improvements: +- Reduced memory overhead by eliminating data conversion +- Faster rendering through direct WebGL texture creation +- Improved handling of large datasets (>100k points)
95-97: Add migration script suggestion.For large codebases, manual migration could be error-prone. Consider providing a migration script or codemods.
Would you like me to help create a migration script that automates these changes?
src/modules/ForceManyBody/force-level.frag (1)
88-117: Consider unrolling the inner loop.The nested loops for calculating additional velocity could benefit from unrolling the inner loop since it has a fixed size of 4 iterations.
This would reduce branch prediction overhead in the GPU.
README.md (1)
11-11: Add alt text to the video element for accessibilityThe video element should include alt text to improve accessibility.
-<img src="https://user-images.githubusercontent.com/755708/173392407-9b05cbb6-d39e-4c2c-ab41-50900cfda823.mp4" autoplay controls> +<img src="https://user-images.githubusercontent.com/755708/173392407-9b05cbb6-d39e-4c2c-ab41-50900cfda823.mp4" + alt="Demo video showing Cosmos graph visualization capabilities" + autoplay controls>🧰 Tools
🪛 Markdownlint (0.37.0)
11-11: null
Images should have alternate text (alt text)(MD045, no-alt-text)
src/modules/Store/index.ts (2)
39-50: Consider adding validation for color valuesThe backgroundColor setter should validate the input color values to ensure they are within the valid range [0,1].
public set backgroundColor (color: [number, number, number, number]) { + // Validate color values are in range [0,1] + if (!color.every(c => c >= 0 && c <= 1)) { + throw new Error('Color values must be in range [0,1]') + } this._backgroundColor = color const brightness = rgbToBrightness(color[0], color[1], color[2]) document.documentElement.style.setProperty('--cosmos-attribution-color', brightness > 0.65 ? 'black' : 'white') if (this.div) this.div.style.backgroundColor = `rgba(${color[0] * 255}, ${color[1] * 255}, ${color[2] * 255}, ${color[3]})` }
74-79: Consider adding boundary checks for space sizeThe scale methods should validate that adjustedSpaceSize is positive to prevent invalid scaling.
public updateScreenSize (width: number, height: number): void { const { adjustedSpaceSize } = this + if (adjustedSpaceSize <= 0) { + throw new Error('Adjusted space size must be positive') + } this.screenSize = [width, height] this.scalePointX .domain([0, adjustedSpaceSize])src/modules/Zoom/index.ts (1)
Line range hint
138-145: Consider documenting the scaling factorsThe magic numbers in the scaling logic (5.0, 1.0, 0.01) should be documented or extracted as named constants.
public convertSpaceToScreenRadius (spaceRadius: number): number { const { config: { scalePointsOnZoom }, store: { maxPointSize }, eventTransform: { k } } = this + // Constants for non-zoom scaling mode + const MAX_SCALE_FACTOR = 5.0 + const MIN_SCALE_FACTOR = 1.0 + const ZOOM_FACTOR = 0.01 let size = spaceRadius * 2 if (scalePointsOnZoom) { size *= k } else { - size *= Math.min(5.0, Math.max(1.0, k * 0.01)) + size *= Math.min(MAX_SCALE_FACTOR, Math.max(MIN_SCALE_FACTOR, k * ZOOM_FACTOR)) } return Math.min(size, maxPointSize) / 2 }CODE_OF_CONDUCT.md (3)
3-4: Fix grammar in the opening sentenceThe subject-verb agreement needs correction.
-Cosmos, as member project of the OpenJS Foundation, use [Contributor Covenant +Cosmos, as a member project of the OpenJS Foundation, uses [Contributor Covenant🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: You might be missing the article “a” here.
Context: # Code of Conduct Cosmos, as member project of the OpenJS Foundation, use [...(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~3-~3: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...ember project of the OpenJS Foundation, use [Contributor Covenant v2.0](https://con...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
19-21: Add missing comma after introductory phraseFor better readability and correct punctuation.
-For reporting issues in spaces related to Cosmos please use the email +For reporting issues in spaces related to Cosmos, please use the email🧰 Tools
🪛 LanguageTool
[typographical] ~19-~19: Consider adding a comma here.
Context: ...rting issues in spaces related to Cosmos please use the email[email protected]. Co...(PLEASE_COMMA)
70-87: Standardize list markersFor consistency with Markdown style guidelines, replace asterisks with dashes in unordered lists.
-* Demonstrating empathy and kindness toward other people +- Demonstrating empathy and kindness toward other people(Apply this change to all list items)
🧰 Tools
🪛 LanguageTool
[style] ~82-~82: Try using a synonym here to strengthen your wording.
Context: ...ind * Trolling, insulting or derogatory comments, and personal or political attacks * Pu...(COMMENT_REMARK)
🪛 Markdownlint (0.37.0)
70-70: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
71-71: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
72-72: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
73-73: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
75-75: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
80-80: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
82-82: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
83-83: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
84-84: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
86-86: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
src/stories/generate-mesh-data.ts (3)
96-96: Remove unnecessary type assertion onlinksarray elements.The
linksarray is of typenumber[], so the type assertionas numberis unnecessary.Apply this diff:
- const sourcePointIndex = links[i * 2] as number + const sourcePointIndex = links[i * 2]
45-46: Use typed arrays forclusterPositionsandpointClustersfor consistency and performance.Consider using
Float32ArrayforclusterPositionsandUint16ArrayforpointClustersto maintain consistency with other typed arrays and potentially improve performance.Apply this diff:
- const pointClusters = new Array(n * m) - const clusterPositions = new Array(nClusters * 2) + const pointClusters = new Uint16Array(n * m) + const clusterPositions = new Float32Array(nClusters * 2)
46-46: Clarify the purpose ofclusterStrength; consider renaming for clarity.The
clusterStrengtharray holds strength values per point rather than per cluster. Consider renaming it topointClusterStrengthor adding comments to explain its purpose and usage.Also applies to: 65-65
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (78)
.eslintrc(1 hunks).gitignore(1 hunks).storybook/main.ts(1 hunks).storybook/manager.ts(1 hunks).storybook/preview.ts(1 hunks)CODE_OF_CONDUCT.md(1 hunks)CONTRIBUTING.md(1 hunks)LICENCE(1 hunks)README.md(2 hunks)SECURITY.md(1 hunks)cosmos-2-0-migration-notes.md(1 hunks)package.json(3 hunks)rollup.config.js(2 hunks)src/attribution.ts(1 hunks)src/config.ts(8 hunks)src/declaration.d.ts(1 hunks)src/helper.ts(2 hunks)src/index.ts(19 hunks)src/modules/Clusters/calculate-centermass.vert(1 hunks)src/modules/Clusters/force-cluster.frag(1 hunks)src/modules/Clusters/index.ts(1 hunks)src/modules/Drag/index.ts(1 hunks)src/modules/ForceCenter/calculate-centermass.vert(1 hunks)src/modules/ForceCenter/force-center.frag(1 hunks)src/modules/ForceCenter/index.ts(1 hunks)src/modules/ForceGravity/force-gravity.frag(1 hunks)src/modules/ForceGravity/index.ts(1 hunks)src/modules/ForceLink/force-spring.ts(2 hunks)src/modules/ForceLink/index.ts(2 hunks)src/modules/ForceManyBody/calculate-level.vert(1 hunks)src/modules/ForceManyBody/force-centermass.frag(2 hunks)src/modules/ForceManyBody/force-level.frag(5 hunks)src/modules/ForceManyBody/index.ts(3 hunks)src/modules/ForceManyBody/quadtree-frag-shader.ts(2 hunks)src/modules/ForceManyBodyQuadtree/calculate-level.vert(1 hunks)src/modules/ForceManyBodyQuadtree/index.ts(1 hunks)src/modules/ForceManyBodyQuadtree/quadtree-frag-shader.ts(3 hunks)src/modules/ForceMouse/force-mouse.frag(1 hunks)src/modules/ForceMouse/index.ts(1 hunks)src/modules/GraphData/index.ts(1 hunks)src/modules/Lines/draw-curve-line.vert(4 hunks)src/modules/Lines/index.ts(1 hunks)src/modules/Points/color-buffer.ts(0 hunks)src/modules/Points/drag-point.frag(1 hunks)src/modules/Points/draw-highlighted.frag(1 hunks)src/modules/Points/draw-highlighted.vert(1 hunks)src/modules/Points/draw-points.frag(1 hunks)src/modules/Points/draw-points.vert(1 hunks)src/modules/Points/fill-sampled-points.frag(1 hunks)src/modules/Points/fill-sampled-points.vert(1 hunks)src/modules/Points/find-hovered-point.vert(2 hunks)src/modules/Points/find-points-on-area-selection.frag(1 hunks)src/modules/Points/index.ts(6 hunks)src/modules/Points/size-buffer.ts(0 hunks)src/modules/Points/track-positions.frag(1 hunks)src/modules/Points/tracked-buffer.ts(0 hunks)src/modules/Points/update-position.frag(1 hunks)src/modules/Shared/buffer.ts(2 hunks)src/modules/Shared/clear.frag(0 hunks)src/modules/Shared/quad.vert(1 hunks)src/modules/Store/index.ts(3 hunks)src/modules/Zoom/index.ts(4 hunks)src/modules/core-module.ts(1 hunks)src/stories/clusters.stories.ts(1 hunks)src/stories/clusters/radial.ts(1 hunks)src/stories/clusters/with-labels.ts(1 hunks)src/stories/clusters/worm.ts(1 hunks)src/stories/create-cluster-labels.ts(1 hunks)src/stories/create-cosmos.ts(1 hunks)src/stories/experiments.stories.ts(1 hunks)src/stories/experiments/full-mesh.ts(1 hunks)src/stories/experiments/mesh-with-holes.ts(1 hunks)src/stories/generate-mesh-data.ts(1 hunks)src/stories/welcome.mdx(1 hunks)src/types.ts(0 hunks)src/variables.ts(1 hunks)tsconfig.json(1 hunks)vite.config.ts(1 hunks)
💤 Files with no reviewable changes (5)
- src/modules/Shared/clear.frag
- src/types.ts
- src/modules/Points/tracked-buffer.ts
- src/modules/Points/size-buffer.ts
- src/modules/Points/color-buffer.ts
✅ Files skipped from review due to trivial changes (7)
- .gitignore
- src/stories/welcome.mdx
- .storybook/main.ts
- src/attribution.ts
- LICENCE
- .storybook/manager.ts
- SECURITY.md
🧰 Additional context used
🪛 LanguageTool
CODE_OF_CONDUCT.md
[uncategorized] ~3-~3: You might be missing the article “a” here.
Context: # Code of Conduct Cosmos, as member project of the OpenJS Foundation, use [...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~3-~3: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...ember project of the OpenJS Foundation, use [Contributor Covenant v2.0](https://con...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[typographical] ~19-~19: Consider adding a comma here.
Context: ...rting issues in spaces related to Cosmos please use the email [email protected]. Co...
(PLEASE_COMMA)
[style] ~23-~23: ‘with regard to’ might be wordy. Consider a shorter alternative.
Context: ...mit to: - maintain the confidentiality with regard to the reporter of an incident - to partic...
(EN_WORDINESS_PREMIUM_WITH_REGARD_TO)
[uncategorized] ~30-~30: Do not mix variants of the same word (‘organization’ and ‘organisation’) within a single text.
Context: ...example, repositories within the OpenJS organization, use the email `[email protected]...
(EN_WORD_COHERENCY)
[style] ~34-~34: ‘with regard to’ might be wordy. Consider a shorter alternative.
Context: ...its to: - maintain the confidentiality with regard to the reporter of an incident - to partic...
(EN_WORDINESS_PREMIUM_WITH_REGARD_TO)
[style] ~42-~42: Consider a shorter alternative to avoid wordiness.
Context: ... the CPC has not been properly handled. In order to escalate to the CoCP send an email to `...
(IN_ORDER_TO_PREMIUM)
[style] ~43-~43: Consider using “email”.
Context: ...ndled. In order to escalate to the CoCP send an email to [email protected]. Fo...
(SEND_AN_EMAIL)
[style] ~82-~82: Try using a synonym here to strengthen your wording.
Context: ...ind * Trolling, insulting or derogatory comments, and personal or political attacks * Pu...
(COMMENT_REMARK)
[uncategorized] ~171-~171: You might be missing the article “the” here.
Context: ...nant.org/version/2/0/code_of_conduct). Community Impact Guidelines were inspired by [Moz...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
README.md
[uncategorized] ~50-~50: Loose punctuation mark.
Context: ...ph.render(); ``` - pointPositions: A Float32Array of `[x1, y1, x2, y2, ......
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~51-~51: Loose punctuation mark.
Context: ...y1, x2, y2, ..., xN, yN]. - **links**: A Float32Array of [sourceIndex1, targe...
(UNLIKELY_OPENING_PUNCTUATION)
cosmos-2-0-migration-notes.md
[uncategorized] ~36-~36: Possible missing comma found.
Context: ...uration Updates Accessor functions for styling such as nodeColor, nodeSize, `linkC...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 Markdownlint (0.37.0)
CODE_OF_CONDUCT.md
70-70: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
71-71: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
72-72: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
73-73: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
75-75: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
80-80: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
82-82: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
83-83: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
84-84: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
86-86: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
6-6: null
Link fragments should be valid
(MD051, link-fragments)
README.md
11-11: null
Images should have alternate text (alt text)
(MD045, no-alt-text)
101-101: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
cosmos-2-0-migration-notes.md
77-77: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
81-81: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
87-87: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
93-93: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 Biome (1.9.4)
vite.config.ts
[error] 50-50: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (63)
src/index.ts (1)
1001-1001: 🛠️ Refactor suggestion
Replace deprecated event.which with event.button
At line 1001, event.which is deprecated and may not be supported in all browsers. Use event.button to detect mouse button clicks for better compatibility.
Apply this diff to update the event property:
- this.isRightClickMouse = event.which === 3
+ this.isRightClickMouse = event.button === 2Likely invalid or redundant comment.
src/declaration.d.ts (1)
3-7: Fix indentation to match existing declarations.
The indentation of the new module declaration doesn't match the existing declarations above it.
declare module '*.frag';
declare module '*.vert';
declare module '*?raw' {
- const content: string
- // eslint-disable-next-line import/no-default-export
- export default content
- }
+ const content: string
+ // eslint-disable-next-line import/no-default-export
+ export default content
+}Let's verify how this new raw import is being used:
✅ Verification successful
Fix indentation to match existing declarations.
The indentation of the new module declaration doesn't match the existing declarations above it. The raw import declaration is actively used in story files for importing code examples.
- Fix indentation from 4 spaces to 2 spaces to maintain consistency with existing declarations
declare module '*.frag';
declare module '*.vert';
declare module '*?raw' {
- const content: string
- // eslint-disable-next-line import/no-default-export
- export default content
- }
+ const content: string
+ // eslint-disable-next-line import/no-default-export
+ export default content
+}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for raw imports usage
rg "from.*\?raw';" -g '!*.d.ts'
Length of output: 36
Script:
#!/bin/bash
# Let's try a broader search for raw imports, including different quote styles and without semicolon
rg "from.*\?raw['\"]" -g '!*.d.ts'
# Also search for potential import() calls with raw
rg "import\(.*\?raw['\"]" -g '!*.d.ts'
# Let's also check if there are any files that end with ?raw being imported
fd ".*\?raw$"
Length of output: 883
src/modules/ForceCenter/calculate-centermass.vert (2)
5-6: LGTM! Verify texture size calculation.
The rename from position to positionsTexture improves clarity. However, let's verify that the pointsTextureSize uniform is consistently named across other shaders.
✅ Verification successful
Texture size uniform naming is consistent across shaders
The verification shows that pointsTextureSize is consistently used across shaders when referring to the points texture size. Other texture size uniforms follow the same pattern: clustersTextureSize for clusters and levelTextureSize for levels. This confirms the naming is standardized and correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for texture size uniform naming consistency
rg "uniform.*TextureSize" -g "*.{frag,vert}"
Length of output: 1296
13-13: Verify texture coordinate calculation.
The texture coordinate calculation looks correct, but let's ensure this pattern is consistent across other shaders using the same texture.
✅ Verification successful
These commands will help us:
- Search for texture2D calls specifically with TextureSize in the calculation
- Examine all vertex shader files
- Examine all fragment shader files
to verify the texture coordinate calculation pattern across shaders.
Based on the examination of all vertex and fragment shaders in the codebase, I can now provide a final response:
Texture coordinate calculation is consistent across shaders.
The pattern texture2D(positionsTexture, pointIndices / pointsTextureSize) is consistently used across multiple shaders when accessing point positions, with minor variations:
- Some shaders add a 0.5 offset:
(pointIndices + 0.5) / pointsTextureSize - Similar pattern is used for other textures like
clusterTexture,centermassTexture
The texture coordinate calculation in the reviewed file follows the established pattern in the codebase and is correct for accessing texture data.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check texture coordinate calculation pattern
ast-grep --pattern 'texture2D($_, $_ / $_TextureSize)'
Length of output: 56
Script:
#!/bin/bash
# Search for texture2D usage patterns in shader files
rg "texture2D.*TextureSize" --type glsl -A 2 -B 2
Length of output: 80
Script:
#!/bin/bash
# Find and examine all vertex shader files
fd "\.vert$" --exec cat {}
Length of output: 12708
Script:
#!/bin/bash
# Find and examine all fragment shader files
fd "\.frag$" --exec cat {}
Length of output: 15003
src/stories/experiments/mesh-with-holes.ts (1)
5-5: Same improvements needed as in full-mesh.ts.
This file would benefit from the same improvements suggested for full-mesh.ts:
- Extracting magic numbers into named constants
- Adding type safety for the destructured object
src/modules/Shared/quad.vert (2)
5-6: Well-documented attribute and varying names.
The renaming from quad/index to vertexCoord/textureCoords is more descriptive and aligns with the Cosmos 2.0 terminology changes. The added comments clearly explain their purpose.
9-11: Verify shader compatibility across the codebase.
The coordinate conversion logic is correct. However, let's verify that other shaders are updated to work with the new textureCoords varying name.
✅ Verification successful
Based on the shader files content, I can now generate the final response:
The shader coordinate system changes are properly synchronized across the codebase.
The textureCoords varying is consistently used across all relevant shader pairs:
- In
quad.vert, it's correctly computed from vertex coordinates - Fragment shaders that need texture sampling (e.g., forces.frag, mouse.frag, position.frag) properly use the
textureCoordsvarying - No shaders are using any old/incompatible coordinate naming
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for shaders still using the old 'index' varying
rg --type glsl "varying.*index"
# Search for fragment shaders that might need the new textureCoords
rg --type glsl "textureCoords"
Length of output: 122
Script:
#!/bin/bash
# Search for files with .vert or .frag extensions (common shader file extensions)
fd "\.(vert|frag)$" -x cat {} \;
Length of output: 27686
src/modules/Points/draw-highlighted.frag (1)
6-7: LGTM! Variable renaming aligns with project conventions.
The renaming from pos to vertexPosition and particleOpacity to pointOpacity is consistent with the project's move from nodes/particles to points terminology.
src/modules/Points/drag-point.frag (1)
1-4: LGTM! Appropriate precision handling for GL ES.
The shader correctly handles precision requirements for both GL ES and non-GL ES environments.
src/modules/Points/track-positions.frag (1)
12-14: LGTM! Efficient implementation with early discard.
The shader efficiently handles untracked points using early discard and correctly calculates texture coordinates for position lookup. The + 0.5 offset ensures proper texel center sampling.
src/modules/Points/update-position.frag (1)
5-5: LGTM! Variable renames improve clarity
The renaming of variables (position → positionsTexture, index → textureCoords) better reflects their purpose and aligns with the new point-centric terminology.
Also applies to: 10-10, 13-14
✅ Verification successful
Variable naming is consistently applied across shader files
The verification shows that the variable names positionsTexture and textureCoords are consistently used across all shader files in the codebase:
positionsTextureis used uniformly as a sampler2D uniform for accessing point positionstextureCoordsis used consistently as a varying variable for texture coordinates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent naming across other shader files
rg -g '*.frag' -g '*.vert' 'positionsTexture|textureCoords' --stats
Length of output: 5990
src/modules/ForceManyBody/calculate-level.vert (1)
5-5: LGTM! Variable renames improve clarity
The renaming of variables (position → positionsTexture, indexes → pointIndices) aligns with the point-centric terminology and improves code clarity.
Also applies to: 10-10
src/modules/ForceManyBodyQuadtree/calculate-level.vert (2)
5-10: LGTM! Consistent naming improvements
The renaming of uniforms and attributes (position → positionsTexture, indexes → pointIndices) aligns well with the project-wide terminology shift and improves clarity by being more descriptive about the data being handled.
15-15: Verify texture sampling performance
The texture sampling operation looks correct, but since this is a performance-critical shader code:
✅ Verification successful
Texture sampling pattern is consistent across shaders
The texture sampling operation in calculate-level.vert follows the same pattern used consistently throughout the codebase. The search reveals two main sampling patterns:
texture2D(positionsTexture, textureCoords)for fragment shaderstexture2D(positionsTexture, pointIndices / pointsTextureSize)for vertex shaders (with minor variations using offsets)
This consistent implementation suggests the texture sampling is already optimized for the specific use cases in vertex and fragment shaders.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other texture sampling patterns in shaders that might need similar optimizations
rg -t glsl "texture2D.*positionsTexture" --type-add 'glsl:*.{vert,frag}'
Length of output: 2509
src/modules/ForceMouse/force-mouse.frag (1)
5-5: LGTM: Consistent naming improvements
The renaming of position to positionsTexture and index to textureCoords improves clarity and aligns with the new point-centric terminology.
Also applies to: 9-9
src/modules/ForceGravity/force-gravity.frag (1)
5-5: LGTM: Consistent naming improvements
The renaming of uniforms and varying variables maintains consistency with the new terminology across shaders.
Also applies to: 10-10
src/modules/core-module.ts (2)
Line range hint 14-26: LGTM: Constructor parameter types align with property types
The constructor properly initializes all properties with their new non-generic types, maintaining type safety.
7-12: Verify impact of removing generic type parameters
The removal of generic type parameters N and L is a breaking change that simplifies the type system. This aligns with the architectural shift from nodes to points in Cosmos 2.0.
✅ Verification successful
Let me analyze the usage patterns and type parameter dependencies to better understand the impact of removing the generic parameters.
Removal of generic type parameters is safe
The removal of generic type parameters N and L from CoreModule is a safe change as part of the architectural shift to Cosmos 2.0. Evidence from the codebase shows:
- All inheriting classes (
Points,Lines,ForceMouse,ForceManyBody, etc.) useCoreModulewithout type parameters - No generic type usage patterns found in the inheritance chain
- The change aligns with the standardization from nodes/links to points-based architecture
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining generic type usage that might need updating
rg -g '*.ts' -g '*.tsx' '<[^>]*CosmosInputNode[^>]*>' .
rg -g '*.ts' -g '*.tsx' '<[^>]*CosmosInputLink[^>]*>' .
# Search for potential extension points that might be affected
ast-grep --pattern 'extends CoreModule'
Length of output: 157
Script:
#!/bin/bash
# Search for references to CoreModule and its usage patterns
rg -g '*.ts' -g '*.tsx' 'CoreModule' -A 2 -B 2
# Search for any type parameters in the codebase that might be affected
ast-grep --pattern 'class $_ <$_>'
# Look for imports of CoreModule to understand usage
rg -g '*.ts' -g '*.tsx' "import.*CoreModule"
Length of output: 7848
src/modules/ForceCenter/force-center.frag (2)
5-7: LGTM: Improved uniform variable naming
The renamed uniforms (positionsTexture, centermassTexture, centerForce) are more descriptive and consistent with the project's naming conventions.
14-24: Verify shader calculations remain unchanged
The shader logic appears correct, but let's verify that the renaming didn't introduce any unintended changes to the force calculations.
✅ Verification successful
Let me gather more specific information about the force center calculations.
Force calculations are consistent with other force implementations
The shader calculations remain unchanged and follow the same patterns as other force implementations in the codebase:
- The center mass position calculation (
centermassPosition = centermassValues.xy / centermassValues.b) matches the pattern used in ForceManyBody shaders - The force calculation using
alpha * centerForce * distfollows similar patterns seen in other force shaders (ForceGravity, Clusters) - The angle-based velocity application using
cos/sinis consistent with other force implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related shaders to verify consistent variable naming
rg -g '*.frag' -g '*.vert' 'uniform\s+sampler2D\s+position' .
rg -g '*.frag' -g '*.vert' 'uniform\s+sampler2D\s+positionsTexture' .
# Check for any remaining old variable names
rg -g '*.frag' -g '*.vert' '\b(position|centermass|center)\b' .
Length of output: 5668
Script:
#!/bin/bash
# Check the force center calculations in related files
rg -g '*.frag' -g '*.vert' -A 10 'centermassPosition.*=.*centermass' .
# Check for any alpha and centerForce usage
rg -g '*.frag' -g '*.vert' '(alpha|centerForce).*\*.*dist' .
# Look for similar force calculation patterns
rg -g '*.frag' -g '*.vert' 'addV.*=.*force.*dist' .
Length of output: 3291
src/modules/Clusters/calculate-centermass.vert (1)
18-22: Verify cluster index bounds checking
The cluster index validation is good, but consider adding an upper bound check to ensure indices don't exceed texture dimensions.
src/modules/Points/draw-points.frag (2)
7-9: LGTM! Clean separation of color and alpha components.
The introduction of separate rgbColor and alpha varying variables improves modularity and makes the code more maintainable.
15-23: LGTM! Clear and well-documented point rendering logic.
The improved variable names (pointCoord, pointCenterDistance) and added comments make the code more self-documenting. The smoothstep calculation for opacity is well-structured.
src/modules/Points/fill-sampled-points.vert (1)
5-9: LGTM! Improved naming clarity for uniforms.
The renaming of position to positionsTexture and transform to transformationMatrix better reflects their purposes.
tsconfig.json (1)
7-8: Verify build configuration alignment.
The new path mappings look good, but ensure they're properly configured in the build system (e.g., Rollup/Webpack) to maintain consistency between development and production environments.
✅ Verification successful
Path mappings are correctly configured across build tools
The path mappings for @cosmograph/cosmos are properly aligned across all build configurations:
rollup.config.js: Usespath.resolve(__dirname, 'src/')vite.config.ts: Usesresolve(__dirname, 'src/')tsconfig.json: Maps to["src/"]and["src/*"]- Additional TypeScript path transform plugin
@zerollup/ts-transform-pathsis configured to handle the mappings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for corresponding path mappings in build configuration
# Look for build config files
echo "Checking build configuration files..."
fd -e js -e ts -e json -g "*config*" | while read -r file; do
echo "Checking $file for @cosmograph/cosmos path mappings..."
rg -A 3 "@cosmograph/cosmos" "$file"
done
Length of output: 1263
src/modules/ForceMouse/index.ts (1)
7-7: Good simplification of type parameters!
The removal of generic type parameters aligns well with the project's move towards simpler, point-based architecture.
src/modules/ForceGravity/index.ts (2)
7-7: LGTM: Class simplification aligns with architectural changes
The removal of generic type parameters aligns with the broader architectural changes in Cosmos 2.0, making the codebase more maintainable.
12-27: Verify the impact of configuration changes
The initialization logic and naming changes look good, but we should verify the config property rename from simulation.gravity to simulationGravity doesn't break existing configurations.
✅ Verification successful
Let me check the content of config.ts to understand how the old and new config patterns are handled.
Let me check the default config values to complete the verification.
Let me try to find the correct path for the variables file.
Property rename from simulation.gravity to simulationGravity is properly handled
The codebase shows a clean transition from simulation.gravity to simulationGravity:
- The default value is correctly sourced from
defaultConfigValues.simulation.gravityin the config class - All usages in the codebase are using the new
simulationGravityproperty - The config class maintains backward compatibility by mapping the new property to the old default value structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining uses of the old config pattern
rg -g '!*.{md,txt}' -l 'simulation.*gravity'
# Search for examples of the new config pattern
rg -g '!*.{md,txt}' 'simulationGravity'
Length of output: 691
Script:
#!/bin/bash
# Check the content of config.ts to see how the configuration is handled
cat src/config.ts
Length of output: 21288
Script:
#!/bin/bash
# Check the content of the file containing default config values
cat src/graph/variables.ts
Length of output: 83
Script:
#!/bin/bash
# Find the file containing default variables
fd variables
Length of output: 31
Script:
#!/bin/bash
# Check the content of the variables file
cat src/variables.ts
Length of output: 1571
src/modules/Points/find-points-on-area-selection.frag (2)
5-6: LGTM: Uniform variable renames align with new terminology
The renaming of uniforms from node-centric to point-centric terminology (position → positionsTexture, transform → transformationMatrix, scaleNodesOnZoom → scalePointsOnZoom) is consistent with the Cosmos 2.0 architectural changes.
Also applies to: 11-11, 13-13
18-25: Verify point size calculation with zoom levels
The point size calculation logic in pointSizeF looks correct, but we should verify the behavior across different zoom levels, especially with the scalePointsOnZoom flag.
✅ Verification successful
Point size calculation is consistently implemented across shaders
The point size calculation logic is correctly and consistently implemented across all shader files:
find-points-on-area-selection.fragdraw-highlighted.vertdraw-points.vertfind-hovered-point.vert
All implementations follow the same scaling pattern:
- When
scalePointsOnZoomis true: size scales directly with zoom (transformationMatrix[0][0]) - When
scalePointsOnZoomis false: size scales with a clamped factor (min(5.0, max(1.0, zoom * 0.01))) - All calculations are properly capped by
maxPointSize
The configuration is properly managed through the application's config system with appropriate defaults, and the maxPointSize is correctly initialized based on hardware limits.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests that verify point scaling behavior
rg -l "scalePointsOnZoom.*test" --type ts
# Look for related point size calculations in other shaders
fd . -e frag -e vert -x grep -l "pointSize"
Length of output: 186
Script:
#!/bin/bash
# Check the content of the vertex shader to understand point size calculations
cat ./src/modules/Points/draw-highlighted.vert
# Look for any configuration or constants related to point size
rg "maxPointSize|scalePointsOnZoom" -A 2 -B 2
Length of output: 11509
rollup.config.js (2)
7-7: LGTM: TypeScript plugin update simplifies configuration
The switch to @rollup/plugin-typescript and removal of explicit ttypescript configuration aligns with modern TypeScript tooling practices.
Also applies to: 39-39
44-44: Verify module resolution with new alias
The new alias @cosmograph/cosmos needs verification to ensure it doesn't conflict with external package imports.
✅ Verification successful
No conflicts found with @cosmograph/cosmos alias
The alias configuration is safe to use. All imports of @cosmograph/cosmos are from story files within the src directory, which is the intended target of the alias. While the package exists on npm, there are no direct imports from the external package that would conflict with the local alias.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for @cosmograph/cosmos imports
rg "@cosmograph/cosmos" --type ts
# Check package.json for potential conflicts
jq '.dependencies + .devDependencies | keys | .[]' package.json | grep -i cosmos
Length of output: 447
Script:
#!/bin/bash
# Check if there are any imports from node_modules that might conflict
fd -t f -e ts -e js -e json . | xargs grep -l "require.*@cosmograph/cosmos\|from.*@cosmograph/cosmos"
# Check if the package is published on npm
curl -s https://registry.npmjs.org/@cosmograph%2Fcosmos | jq -r '.error // "Package exists"'
Length of output: 313
src/stories/clusters.stories.ts (1)
1-5: LGTM: Clean story component organization
The story imports and type definitions are well-organized, following Storybook best practices.
src/variables.ts (2)
1-3: LGTM: Consistent terminology change from "node" to "point"
The renaming aligns with the major version's architectural changes.
30-41: Verify impact of simulation parameter changes
Significant changes to simulation parameters:
- Increased decay (1000 → 5000)
- Added gravity (0 → 0.25)
- Increased repulsion (0.1 → 1.0)
- Modified repulsionTheta (1.7 → 1.15)
- Increased linkDistance (2 → 10)
- Added cluster (0.1)
These changes could significantly affect the graph layout behavior.
src/modules/ForceManyBody/force-centermass.frag (3)
1-2: LGTM: Improved shader documentation
Clear documentation of the shader's purpose and functionality.
8-8: LGTM: Clearer variable naming
Renaming improves clarity:
position→positionsTextureindex→textureCoords
Also applies to: 16-16
18-19: LGTM: Better function naming
Renaming calcAdd to calculateAdditionalVelocity better describes the function's purpose.
src/modules/Points/draw-points.vert (3)
5-18: LGTM: Enhanced point customization
Good addition of per-point size and color attributes, allowing more flexible styling.
50-56: LGTM: Improved alpha handling
Clean separation of base alpha from greyout status modification.
25-33: Verify point size scaling behavior
The point size calculation has changed significantly:
- New scaling behavior when
scalePointsOnZoomis false - Different ratio application
✅ Verification successful
The previous search failed because the --type test flag wasn't properly configured. I've modified the search to:
- Look for test files with proper type configuration
- Search for all usages of the point size calculation
- Look specifically in shader files for point size related code to understand the context better
Point size scaling changes are consistent across shaders
The changes to point size calculation are consistently implemented across all shader files where it's used:
draw-points.vertfind-hovered-point.vertdraw-highlighted.vert
All implementations follow the same logic:
- When
scalePointsOnZoomis true: size scales directly with zoom (transformationMatrix[0][0]) - When
scalePointsOnZoomis false: size scales with a clamped zoom factor (min(5.0, max(1.0, transformationMatrix[0][0] * 0.01))) - All implementations properly apply the ratio and respect maxPointSize
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests covering point size scaling
rg -l "scalePointsOnZoom.*test|calculatePointSize.*test" --type test
Length of output: 99
Script:
#!/bin/bash
# Search for test files containing point size scaling related tests
rg -l "scalePointsOnZoom|calculatePointSize" --type-add 'test:*{test,spec}.*' --type test
# Search for usages of calculatePointSize function
rg "calculatePointSize" -A 5 -B 5
# Look for shader files with point size calculations
fd ".*\.(vert|frag|glsl)$" --exec rg "pointSize|scalePointsOnZoom" {} -A 5 -B 5
Length of output: 8583
src/helper.ts (1)
50-52: LGTM! Clean and effective implementation.
The function correctly handles all edge cases for number validation.
src/modules/Points/find-hovered-point.vert (1)
22-30: LGTM! Point size calculation is well-implemented.
The function properly handles both scaled and unscaled modes with appropriate size limits.
src/stories/create-cluster-labels.ts (1)
3-5: LGTM! Clean closure pattern for state management.
The function structure effectively manages cluster label state while providing a clean update interface.
src/modules/Points/draw-highlighted.vert (2)
3-20: LGTM: Variable renames align with architectural changes
The renaming of variables (e.g., quad → vertexCoord, particleGreyoutStatus → pointGreyoutStatusTexture) consistently reflects the architectural shift from nodes/particles to points terminology.
47-61: Verify the point size calculation in screen space
The point size calculation and transformation to screen space involves multiple mathematical operations. Please ensure this works correctly with different zoom levels and screen sizes.
✅ Verification successful
Let me try a different approach to verify the point size calculation implementation.
Let me check the vertex shader implementation to understand the point size calculation better.
Based on the shader code analysis, I can now provide a final response:
Point size calculation implementation is correct and well-handled
The point size calculation in screen space is properly implemented with:
- Correct zoom level handling through
scalePointsOnZoomuniform - Size limits enforced via
maxPointSizeuniform - Proper transformation matrix scaling
- Screen size compensation for consistent rendering
- Ring radius adjustment for highlighted points
The implementation accounts for:
- Different zoom levels through
transformationMatrix[0][0] - Device pixel ratio through
sizeScale - Screen space conversion with proper normalization
- Size clamping for both zoomed and non-zoomed states
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related test files or usage examples
rg -g '*.test.*' -g '*.spec.*' "calculatePointSize|pointSize"
# Look for any reported issues related to point sizing
rg -g '*.md' -g '*.txt' "point.*size.*issue|zoom.*issue"
Length of output: 271
Script:
#!/bin/bash
# Search for calculatePointSize function definition
ast-grep --pattern 'function calculatePointSize($_) { $$$ }'
# Search for other vertex shaders that might be handling point sizes
fd -e vert --exec grep -l "pointSize"
# Look for any related configuration or constant definitions
rg "pointSize|sizeScale|relativeRingRadius" --type-add 'config:*.{js,ts,json}' -t config
Length of output: 3452
Script:
#!/bin/bash
# Get the content of the vertex shader
cat src/modules/Points/draw-highlighted.vert
# Look for the calculatePointSize function definition in shader files
fd -e vert -e frag --exec grep -A 5 "calculatePointSize"
# Check for any related uniform declarations
rg "uniform.*size|uniform.*scale" -g "*.vert"
Length of output: 3962
src/modules/ForceLink/force-spring.ts (2)
7-15: Improved texture naming enhances code clarity
The renaming of uniform sampler2D variables follows a consistent pattern and better describes their purpose:
positionsTextureclearly indicates it stores point positionslinkInfoTexturestores first link indices and amountlinkIndicesTexturestores link indiceslinkPropertiesTexturestores link bias and strengthlinkRandomDistanceTexturestores random distance values
29-32: Verify texture coordinate calculations
The code extracts link information from linkInfoTexture using textureCoords. Please ensure that the texture coordinates are correctly mapped when reading:
linkInfo.rfor iCountlinkInfo.gfor jCountlinkInfo.bfor linkAmount
✅ Verification successful
Texture coordinate calculations are correctly implemented
The texture coordinate calculations for the linkInfoTexture are properly implemented and consistent with the codebase's texture sampling patterns. The evidence shows:
- The
textureCoordsvarying is correctly passed from vertex to fragment shader through the standard quad vertex shader (src/modules/Shared/quad.vert). - The texture coordinates are properly normalized from [-1, 1] to [0, 1] range:
textureCoords = (vertexCoord + 1.0) / 2.0 - The sampling of
linkInfoTexturefollows the same pattern used consistently across other force calculations in the codebase (ForceCenter, ForceGravity, etc.) - The RGB components (r, g, b) are correctly mapped to store iCount, jCount, and linkAmount respectively, which is consistent with WebGL texture sampling conventions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of linkInfo texture coordinates to verify consistency
rg -A 3 "texture2D\(.*linkInfo.*\)" --type glsl
Length of output: 78
Script:
#!/bin/bash
# Search for texture coordinate calculations and texture2D usage in all files
rg -l "texture2D"
# After finding the files, let's look at their content
rg "texture2D.*linkInfo|textureCoords.*linkInfo|linkInfo.*texture"
Length of output: 1159
Script:
#!/bin/bash
# Let's examine the full content of force-spring.ts to understand the texture coordinate calculation
cat src/modules/ForceLink/force-spring.ts
# Also search for any related texture coordinate calculations in the codebase
rg "textureCoords" -A 3
Length of output: 12901
src/modules/ForceManyBody/quadtree-frag-shader.ts (1)
52-52: Consistent naming with force-spring.ts
The renaming of variables maintains consistency with force-spring.ts:
position→positionsTextureindex→textureCoords
Also applies to: 59-59
package.json (2)
94-97: ESLint Storybook integration
Added Storybook ESLint configuration. This ensures consistent code style in stories.
34-74: Verify compatibility of new dependencies
Significant additions:
- Storybook ecosystem packages
- Vite and related plugins
- Updated TypeScript and build tools
src/modules/ForceManyBodyQuadtree/quadtree-frag-shader.ts (2)
1-3: LGTM! Clear and informative header comments.
The comments effectively describe the shader's purpose and functionality.
64-69: Verify the distance threshold constant.
The distanceMin2 value of 1.0 is used as a minimum squared distance threshold. Please verify if this value is appropriate for the scale of your coordinate system.
✅ Verification successful
The minimum distance threshold of 1.0 is consistent with the codebase.
The value of 1.0 for distanceMin2 is appropriate because:
- It's consistently used across all force calculation shaders in the codebase (ForceManyBody and ForceManyBodyQuadtree modules)
- The codebase uses a normalized coordinate system with a configurable
spaceSize(default 8192) and proper scaling mechanisms - The distance threshold serves as a numerical stability measure to prevent division by zero or extremely small values in force calculations, rather than being tied to the visual coordinate system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other distance thresholds in shaders
rg -A 2 "distanceMin|MIN_DISTANCE" --type glsl
Length of output: 77
Script:
#!/bin/bash
# Search for distance-related constants in TypeScript/JavaScript files
rg -A 2 "distanceMin|MIN_DISTANCE"
# Search for shader files and their content
fd -e ts -e js -X rg -l "shader|\.glsl|\.frag"
# Look for any configuration or constant files
fd "config|constant" -e ts -e js
Length of output: 1929
Script:
#!/bin/bash
# Check the config file for any distance-related configurations
cat src/config.ts
# Look for any scale or coordinate system configurations
rg -A 2 "scale|coordinate|bounds|dimension" src/config.ts src/modules/ForceManyBodyQuadtree/
# Check the usage context of the quadtree shader
rg -A 5 "quadtree-frag-shader" src/modules/ForceManyBodyQuadtree/
Length of output: 23227
src/modules/Lines/draw-curve-line.vert (3)
6-8: LGTM: Uniform variable names updated for clarity and consistency
The renaming of uniforms improves code clarity:
positions→positionsTextureparticleGreyoutStatus→pointGreyoutStatustransform→transformationMatrix
45-46: LGTM: Texture sampling updated to use new uniform names
The texture sampling code has been correctly updated to use the renamed uniforms while maintaining the same functionality.
Also applies to: 48-49
60-60: LGTM: Transformation matrix usage updated consistently
All occurrences of the transformation matrix have been updated to use transformationMatrix consistently throughout the shader.
Also applies to: 67-67, 80-80, 104-104
src/modules/ForceCenter/index.ts (2)
19-35: LGTM: Improved resource initialization with null checks
The create() method now properly checks for existing resources before initialization, preventing potential resource leaks.
40-97: LGTM: Enhanced command initialization with defensive programming
The initPrograms() method has been improved with:
- Null checks before command creation
- Updated attribute names for consistency
- Proper usage of pointsNumber instead of nodes.length
README.md (2)
31-47: LGTM: Updated code example reflects new API
The code example accurately demonstrates the new API with:
- Updated import statements
- New method names (setPointPositions, setLinks)
- Updated configuration properties
- Point-based terminology
58-67: LGTM: Comprehensive v2.0 changes documentation
The "What's New in v2.0?" section effectively communicates:
- Data structure changes
- API updates
- New features
- Breaking changes
src/modules/Store/index.ts (1)
104-107: LGTM: Clean implementation of setFocusedPoint
The method is concise and handles both setting and clearing the focused point correctly.
src/modules/Zoom/index.ts (1)
66-74: Good addition of extent adjustment logic
The code correctly handles the edge case where x or y extent values are equal by adding a small offset, preventing division by zero in scale calculations.
src/stories/generate-mesh-data.ts (1)
59-60: Verify the narrow range used for generating point positions.
Points are generated within a very narrow range near the center of the space (0.495 to 0.505 of spaceSize). Is this intended to create a clustered effect? If a wider distribution is desired, consider adjusting or parameterizing these values.
| float dist = sqrt(dot(distVector, distVector)); | ||
| if (dist > 0.0) { | ||
| float angle = atan(distVector.y, distVector.x); | ||
| float addV = alpha * dist * clusterCoefficient * clusterCustomCoeff.r; | ||
| velocity.rg += addV * vec2(cos(angle), sin(angle)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize vector operations to improve performance
The current implementation calculates the angle using atan and then computes cos and sin functions, which can be computationally expensive. You can optimize this by normalizing the distance vector and using it directly, avoiding the need for trigonometric functions.
Apply this diff to implement the optimization:
- float dist = sqrt(dot(distVector, distVector));
+ float dist = length(distVector);
if (dist > 0.0) {
- float angle = atan(distVector.y, distVector.x);
float addV = alpha * dist * clusterCoefficient * clusterCustomCoeff.r;
- velocity.rg += addV * vec2(cos(angle), sin(angle));
+ vec2 direction = distVector / dist;
+ velocity.rg += addV * direction;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| float dist = sqrt(dot(distVector, distVector)); | |
| if (dist > 0.0) { | |
| float angle = atan(distVector.y, distVector.x); | |
| float addV = alpha * dist * clusterCoefficient * clusterCustomCoeff.r; | |
| velocity.rg += addV * vec2(cos(angle), sin(angle)); | |
| } | |
| float dist = length(distVector); | |
| if (dist > 0.0) { | |
| float addV = alpha * dist * clusterCoefficient * clusterCustomCoeff.r; | |
| vec2 direction = distVector / dist; | |
| velocity.rg += addV * direction; | |
| } |
| } | ||
|
|
||
| if (config?.build?.lib && config.build.rollupOptions && config.build.lib.formats && config.build.lib.formats.includes('umd')) { | ||
| delete config.build.rollupOptions.external |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using the delete operator to improve performance
The use of the delete operator can negatively impact performance because it changes the structure of the object, which may deoptimize JavaScript engine optimizations. Instead of deleting the property, consider setting it to undefined or an empty array.
Apply this diff to adjust the code:
if (config?.build?.lib && config.build.rollupOptions && config.build.lib.formats && config.build.lib.formats.includes('umd')) {
- delete config.build.rollupOptions.external
+ config.build.rollupOptions.external = undefined
}Alternatively, if setting to undefined does not achieve the desired effect, you can assign an empty array:
if (config?.build?.lib && config.build.rollupOptions && config.build.lib.formats && config.build.lib.formats.includes('umd')) {
- delete config.build.rollupOptions.external
+ config.build.rollupOptions.external = []
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 50-50: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
| private levelsTextures = new Map<string, regl.Texture2D>() | ||
| private levelsFbos = new Map<string, regl.Framebuffer2D>() | ||
| private clearLevelsCommand: regl.DrawCommand | undefined | ||
| private calculateLevelsCommand: regl.DrawCommand | undefined | ||
| private quadtreeCommand: regl.DrawCommand | undefined | ||
| private quadtreeLevels = 0 | ||
| private randomValuesTexture: regl.Texture2D | undefined | ||
| private pointIndices: regl.Buffer | undefined | ||
|
|
||
| public create (): void { | ||
| const { reglInstance, store } = this | ||
| if (!store.pointsTextureSize) return | ||
| this.quadtreeLevels = Math.log2(store.adjustedSpaceSize) | ||
| for (let i = 0; i < this.quadtreeLevels; i += 1) { | ||
| const levelTextureSize = Math.pow(2, i + 1) | ||
| this.levelsFbos.set(`level[${i}]`, reglInstance.framebuffer({ | ||
| color: reglInstance.texture({ | ||
| const levelKey = `level[${i}]` | ||
| if (!this.levelsTextures.has(levelKey)) { | ||
| this.levelsTextures.set(levelKey, reglInstance.texture()) | ||
| } | ||
| const texture = this.levelsTextures.get(levelKey) | ||
| if (texture) { | ||
| texture({ | ||
| data: new Float32Array(levelTextureSize * levelTextureSize * 4), | ||
| shape: [levelTextureSize, levelTextureSize, 4], | ||
| type: 'float', | ||
| }), | ||
| depth: false, | ||
| stencil: false, | ||
| }) | ||
| ) | ||
| }) | ||
| } | ||
|
|
||
| if (!this.levelsFbos.has(levelKey)) { | ||
| this.levelsFbos.set(levelKey, reglInstance.framebuffer({ | ||
| color: this.levelsTextures.get(levelKey), | ||
| depth: false, | ||
| stencil: false, | ||
| })) | ||
| } | ||
| } | ||
|
|
||
| // Create random number to prevent point to stick together in one coordinate | ||
| const randomValuesState = new Float32Array(store.pointsTextureSize * store.pointsTextureSize * 4) | ||
| for (let i = 0; i < store.pointsTextureSize * store.pointsTextureSize; ++i) { | ||
| randomValuesState[i * 4] = store.getRandomFloat(-1, 1) * 0.00001 | ||
| randomValuesState[i * 4 + 1] = store.getRandomFloat(-1, 1) * 0.00001 | ||
| } | ||
|
|
||
| this.randomValuesFbo = reglInstance.framebuffer({ | ||
| color: reglInstance.texture({ | ||
| data: randomValuesState, | ||
| shape: [store.pointsTextureSize, store.pointsTextureSize, 4], | ||
| type: 'float', | ||
| }), | ||
| depth: false, | ||
| stencil: false, | ||
| if (!this.randomValuesTexture) this.randomValuesTexture = reglInstance.texture() | ||
| this.randomValuesTexture({ | ||
| data: randomValuesState, | ||
| shape: [store.pointsTextureSize, store.pointsTextureSize, 4], | ||
| type: 'float', | ||
| }) | ||
| if (!this.randomValuesFbo) { | ||
| this.randomValuesFbo = reglInstance.framebuffer({ | ||
| color: this.randomValuesTexture, | ||
| depth: false, | ||
| stencil: false, | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
Ensure proper resource cleanup after removing the destroy method
The removal of the destroy method means that resources like framebuffers, textures, and buffers may not be properly released, potentially leading to memory leaks. It's important to ensure that all allocated resources are cleaned up when they are no longer needed.
Consider reintroducing a cleanup mechanism to dispose of resources. For example, you can add a destroy method or integrate resource cleanup into an existing lifecycle method. Here's a suggested implementation:
public destroy (): void {
this.levelsFbos.forEach(fbo => fbo.destroy())
this.levelsTextures.forEach(texture => texture.destroy())
this.randomValuesFbo?.destroy()
this.randomValuesTexture?.destroy()
this.pointIndices?.destroy()
}Call this destroy method when the module is no longer in use to prevent memory leaks.
src/modules/Lines/index.ts
Outdated
| public updatePointsBuffer (): void { | ||
| const { reglInstance, data, store } = this | ||
| if (data.linksNumber === undefined || data.links === undefined) return | ||
| const instancePoints: [number, number][] = [] // new Float32Array(data.linksNumber * 2) | ||
| for (let i = 0; i < data.linksNumber; i++) { | ||
| const fromIndex = data.links[i * 2] as number | ||
| const toIndex = data.links[i * 2 + 1] as number | ||
| const fromX = fromIndex % store.pointsTextureSize | ||
| const fromY = Math.floor(fromIndex / store.pointsTextureSize) | ||
| const toX = toIndex % store.pointsTextureSize | ||
| const toY = Math.floor(toIndex / store.pointsTextureSize) | ||
| instancePoints[i * 2] = [fromX, fromY] | ||
| instancePoints[i * 2 + 1] = [toX, toY] | ||
| } | ||
|
|
||
| if (!this.pointsBuffer) this.pointsBuffer = reglInstance.buffer(0) | ||
| this.pointsBuffer(instancePoints) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor updatePointsBuffer to use typed arrays for performance
Currently, instancePoints is an array of arrays, which can be less efficient. Refactoring to use a Float32Array can improve performance and reduce memory overhead.
Apply this diff to refactor the code:
- const instancePoints: [number, number][] = [] // new Float32Array(data.linksNumber * 2)
+ const instancePoints = new Float32Array(data.linksNumber * 4)
for (let i = 0; i < data.linksNumber; i++) {
const fromIndex = data.links[i * 2] as number
const toIndex = data.links[i * 2 + 1] as number
const fromX = fromIndex % store.pointsTextureSize
const fromY = Math.floor(fromIndex / store.pointsTextureSize)
const toX = toIndex % store.pointsTextureSize
const toY = Math.floor(toIndex / store.pointsTextureSize)
- instancePoints[i * 2] = [fromX, fromY]
- instancePoints[i * 2 + 1] = [toX, toY]
+ const offset = i * 4
+ instancePoints[offset] = fromX
+ instancePoints[offset + 1] = fromY
+ instancePoints[offset + 2] = toX
+ instancePoints[offset + 3] = toY
}
if (!this.pointsBuffer) this.pointsBuffer = reglInstance.buffer(0)
- this.pointsBuffer(instancePoints)
+ this.pointsBuffer({
+ data: instancePoints,
+ type: 'float',
+ usage: 'dynamic',
+ })Additionally, adjust the attribute settings in initPrograms to match the new buffer format.
Committable suggestion skipped: line range outside the PR's diff.
src/modules/ForceLink/index.ts
Outdated
| const bias = degree / (degree + connectedDegree) | ||
| let strength = data.linkStrength?.[initialLinkIndex] ?? (1 / Math.min(degree, connectedDegree)) | ||
| strength = Math.sqrt(strength) | ||
| linkBiasAndStrengthState[linkIndex * 4 + 0] = bias | ||
| linkBiasAndStrengthState[linkIndex * 4 + 1] = strength | ||
| linkDistanceState[linkIndex * 4] = this.store.getRandomFloat(0, 1) |
There was a problem hiding this comment.
Prevent division by zero in bias and strength calculations
In the calculation of bias and strength, there is a risk of division by zero when degree and connectedDegree are both zero. This can cause runtime errors.
Apply this diff to add safeguards:
const degree = data.degree?.[connectedPointIndex] ?? 0
const connectedDegree = data.degree?.[pointIndex] ?? 0
+ const degreeSum = degree + connectedDegree
+ // Prevent division by zero
+ const bias = degreeSum !== 0 ? degree / degreeSum : 0.5
- let strength = data.linkStrength?.[initialLinkIndex] ?? (1 / Math.min(degree, connectedDegree))
+ const minDegree = Math.min(degree, connectedDegree)
+ // Prevent division by zero
+ let strength = data.linkStrength?.[initialLinkIndex] ?? (1 / Math.max(minDegree, 1))This ensures that division by zero is avoided and provides default values when necessary.
Committable suggestion skipped: line range outside the PR's diff.
| to. Please read [the full text](https://github.com/cosmograph-org/cosmos/CODE_OF_CONDUCT.md) so that you can understand | ||
| what actions will and will not be tolerated. |
There was a problem hiding this comment.
Fix broken Code of Conduct link
The link to CODE_OF_CONDUCT.md appears incorrect. It should be relative to the repository root.
-Please read [the full text](https://github.com/cosmograph-org/cosmos/CODE_OF_CONDUCT.md) so that you can understand
+Please read [the full text](./CODE_OF_CONDUCT.md) so that you can understand📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| to. Please read [the full text](https://github.com/cosmograph-org/cosmos/CODE_OF_CONDUCT.md) so that you can understand | |
| what actions will and will not be tolerated. | |
| to. Please read [the full text](./CODE_OF_CONDUCT.md) so that you can understand | |
| what actions will and will not be tolerated. |
| const clusterPosition = clusterPositions.slice(i * 2, i * 2 + 2) | ||
| const x = clusterPosition[0] | ||
| const y = clusterPosition[1] | ||
| const clusterLabelDiv = clusterLabelDivs[i] as HTMLDivElement | ||
| const screenXY = graph.spaceToScreenPosition([x ?? 0, y ?? 0]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve null handling in coordinate extraction.
The current implementation could be more robust in handling potential null coordinates.
- const clusterPosition = clusterPositions.slice(i * 2, i * 2 + 2)
- const x = clusterPosition[0]
- const y = clusterPosition[1]
- const clusterLabelDiv = clusterLabelDivs[i] as HTMLDivElement
- const screenXY = graph.spaceToScreenPosition([x ?? 0, y ?? 0])
+ const [x, y] = clusterPositions.slice(i * 2, i * 2 + 2)
+ if (x === null || y === null) {
+ // Hide label when coordinates are invalid
+ clusterLabelDivs[i].style.display = 'none'
+ continue
+ }
+ clusterLabelDivs[i].style.display = ''
+ const screenXY = graph.spaceToScreenPosition([x, y])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const clusterPosition = clusterPositions.slice(i * 2, i * 2 + 2) | |
| const x = clusterPosition[0] | |
| const y = clusterPosition[1] | |
| const clusterLabelDiv = clusterLabelDivs[i] as HTMLDivElement | |
| const screenXY = graph.spaceToScreenPosition([x ?? 0, y ?? 0]) | |
| const [x, y] = clusterPositions.slice(i * 2, i * 2 + 2) | |
| if (x === null || y === null) { | |
| // Hide label when coordinates are invalid | |
| clusterLabelDivs[i].style.display = 'none' | |
| continue | |
| } | |
| clusterLabelDivs[i].style.display = '' | |
| const screenXY = graph.spaceToScreenPosition([x, y]) |
| export class ForceCenter extends CoreModule { | ||
| private centermassFbo: regl.Framebuffer2D | undefined | ||
| private clearCentermassCommand: regl.DrawCommand | undefined | ||
| private calculateCentermassCommand: regl.DrawCommand | undefined | ||
| private runCommand: regl.DrawCommand | undefined | ||
| private centermassTexture: regl.Texture2D | undefined | ||
| private pointIndices: regl.Buffer | undefined |
There was a problem hiding this comment.
Consider adding destroy() method for proper cleanup
The removal of the destroy() method might lead to memory leaks. WebGL resources (textures, framebuffers, buffers) should be properly disposed of when no longer needed.
Consider adding a destroy method:
+ public destroy(): void {
+ this.centermassTexture?.destroy()
+ this.centermassFbo?.destroy()
+ this.pointIndices?.destroy()
+ this.centermassTexture = undefined
+ this.centermassFbo = undefined
+ this.pointIndices = undefined
+ }Committable suggestion skipped: line range outside the PR's diff.
| const radius = scaleLinear(radialness) | ||
| radius.domain([0, nClusters]) |
There was a problem hiding this comment.
Fix incorrect usage of scaleLinear when setting domain and range.
The scaleLinear function should be initialized without arguments and configured using the .domain() and .range() methods. Passing radialness directly to scaleLinear is incorrect and may lead to unexpected behavior.
Apply this diff to correct the usage:
- const radius = scaleLinear(radialness)
- radius.domain([0, nClusters])
+ const radius = scaleLinear()
+ .domain([0, nClusters])
+ .range(radialness)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const radius = scaleLinear(radialness) | |
| radius.domain([0, nClusters]) | |
| const radius = scaleLinear() | |
| .domain([0, nClusters]) | |
| .range(radialness) |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
.github/github_pages.yml (1)
15-19: Consider enabling cancellation of in-progress deployments.Setting
cancel-in-progress: truewould prevent potential race conditions and ensure that only the latest changes are being deployed. This is particularly important for documentation deployments where you typically want to show the most recent content.concurrency: group: "pages" - cancel-in-progress: false + cancel-in-progress: true.github/SECURITY.md (2)
5-5: Consider clarifying version support detailsWhile stating "latest release only" is clear, it would be helpful to specify if this includes the latest alpha/beta releases (like 2.0.0-alpha.8 mentioned in the PR summary) or only stable releases.
11-11: Consider enhancing the security reporting linkThe security advisory link could be more specific by including the base URL for users who might be viewing this in contexts where the link isn't clickable.
-Please disclose it at [security advisory](https://github.com/cosmograph-org/cosmos/security/advisories/new). +Please disclose it at our [security advisory page](https://github.com/cosmograph-org/cosmos/security/advisories/new) (https://github.com/cosmograph-org/cosmos/security/advisories/new).README.md (2)
50-51: Enhance data structure documentationWhile the format is clear, consider adding important details about:
- Memory requirements and limitations
- Performance implications of Float32Array
- Whether the arrays need to be aligned or padded
Add a note like:
- **`pointPositions`**: A Float32Array of `[x1, y1, x2, y2, ..., xN, yN]`. - **`links`**: A Float32Array of `[sourceIndex1, targetIndex1, ..., sourceIndexN, targetIndexN]`. + **`pointPositions`**: A Float32Array of `[x1, y1, x2, y2, ..., xN, yN]`. Each point requires 2 * 4 bytes of memory. + **`links`**: A Float32Array of `[sourceIndex1, targetIndex1, ..., sourceIndexN, targetIndexN]`. Each link requires 2 * 4 bytes of memory. + + > **Note**: Using Float32Array provides optimal performance for WebGL operations. No padding is required.🧰 Tools
🪛 LanguageTool
[uncategorized] ~50-~50: Loose punctuation mark.
Context: ...ph.render(); ``` -pointPositions: A Float32Array of `[x1, y1, x2, y2, ......(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~51-~51: Loose punctuation mark.
Context: ...y1, x2, y2, ..., xN, yN]. - **links**: A Float32Array of[sourceIndex1, targe...(UNLIKELY_OPENING_PUNCTUATION)
101-101: Enhance license sectionThe license section should provide more details and a link to the full license text.
Expand the section:
-**MIT** +### License + +This project is licensed under the MIT License - see the [LICENSE](LICENSE) file for details. + +Copyright (c) 2024 Cosmograph🧰 Tools
🪛 Markdownlint (0.37.0)
101-101: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/SECURITY.md(1 hunks).github/github_pages.yml(1 hunks).gitignore(1 hunks)CITATION.cff(1 hunks)LICENCE(1 hunks)README.md(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
🪛 LanguageTool
.github/SECURITY.md
[style] ~9-~9: Consider using a different verb for a more formal wording.
Context: ... This gives us time to work with you to fix the issue before public exposure, reduc...
(FIX_RESOLVE)
README.md
[uncategorized] ~50-~50: Loose punctuation mark.
Context: ...ph.render(); ``` - pointPositions: A Float32Array of `[x1, y1, x2, y2, ......
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~51-~51: Loose punctuation mark.
Context: ...y1, x2, y2, ..., xN, yN]. - **links**: A Float32Array of [sourceIndex1, targe...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 Markdownlint (0.37.0)
README.md
11-11: null
Images should have alternate text (alt text)
(MD045, no-alt-text)
101-101: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (5)
.github/github_pages.yml (2)
1-14: LGTM! Well-structured workflow configuration with appropriate permissions.
The workflow trigger configuration and permissions are well-defined, following security best practices by implementing the principle of least privilege.
40-50: LGTM! Well-configured deployment job.
The deployment configuration is clean and follows GitHub Pages deployment best practices.
CITATION.cff (2)
12-13: Please verify the release date
The release date is set to December 20, 2024, which is in the future. Consider updating it to the actual release date.
21-23: LGTM: License and keyword updates
The license update to MIT and addition of the "Embedding" keyword align well with the project's evolution to version 2.0.
LICENCE (1)
3-3: Verify copyright notice consistency
The copyright notice format is good, but let's ensure it's consistent across all project files.
✅ Verification successful
Copyright notice is consistent across the codebase
The search results show that the only other occurrences of "copyright" are part of the standard MIT license boilerplate text in the LICENSE file itself. There are no inconsistent copyright notices found in other files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any inconsistent copyright notices
rg -i "copyright" --type-not md | grep -v "Contributors to the Cosmos project"
Length of output: 244
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/github_pages.yml (3)
1-6: Consider adding pull_request trigger for preview deploymentsAdding pull_request trigger would enable preview deployments for PRs, making it easier to review visual changes before merging.
on: push: branches: next + pull_request: + branches: next
13-16: Consider enabling cancellation of outdated deploymentsSetting
cancel-in-progress: truewould automatically cancel outdated deployments when new changes are pushed, reducing resource usage and ensuring only the latest changes are deployed.concurrency: group: "pages" - cancel-in-progress: false + cancel-in-progress: true
37-39: Consider combining npm commandsThe npm installation steps can be combined to reduce the number of actions and potential points of failure.
- - run: npm install -g npm@10 - - run: npm ci - - run: npm run build:storybook -- -o _site + - name: Install dependencies and build + run: | + npm install -g npm@10 + npm ci + npm run build:storybook -- -o _site
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/github_pages.yml(1 hunks).gitignore(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/github_pages.yml
[error] 28-28: syntax error: mapping values are not allowed here
(syntax)
🔇 Additional comments (3)
.github/github_pages.yml (3)
8-11: LGTM! Permissions follow security best practices
The permissions configuration follows the principle of least privilege by only granting necessary access levels.
27-33: LGTM! Dependency caching is properly configured
The cache configuration follows best practices by:
- Using the package-lock.json hash as the cache key
- Including a fallback restore-key
- Caching the npm cache directory
🧰 Tools
🪛 yamllint (1.35.1)
[error] 28-28: syntax error: mapping values are not allowed here
(syntax)
44-53: LGTM! Deployment configuration follows best practices
The deployment job is well-configured with proper dependencies, environment settings, and official GitHub actions.
rokotyan
force-pushed
the
next
branch
2 times, most recently
from
57d73d1 to
1c89641
Compare
rokotyan
had a problem deploying
to
github-pages
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
README.md (2)
67-67:⚠️ Potential issueFix broken migration guide link
The migration guide link is incorrect and needs to be updated.
-Check the [Migration Guide](https://github.com/cosmograph-org/cosmos/cosmos-2-0-migration-notes.md) for details. +Check the [Migration Guide](https://github.com/cosmograph-org/cosmos/blob/main/cosmos-2-0-migration-notes.md) for details.
87-87: 🛠️ Refactor suggestionEnhance iOS limitation documentation
The iOS WebGL limitation description should provide more context and potential workarounds.
🧹 Nitpick comments (6)
.github/SECURITY.md (2)
3-5: Clarify version support policy for Cosmos 2.0Given this is a major version upgrade (Cosmos 2.0), consider being more explicit about version support:
## Supported Versions -Security updates are applied only to the latest release. +Security updates are applied only to the latest 2.x release. Version 1.x is no longer supported.
13-13: Consider adding response timeline detailsWhile the 90-day timeline is good, consider adding more details about the response process:
-This project is maintained by a team of volunteers on a reasonable-effort basis. As such, please give us at least 90 days to work on a fix before public exposure. +## Response Timeline + +This project is maintained by a team of volunteers on a reasonable-effort basis. Our typical response process is: + +1. Initial response: Within 48 hours +2. Assessment: Within 1 week +3. Fix development: Up to 90 days + +Please allow at least 90 days before any public disclosure to ensure adequate time for addressing the vulnerability..github/workflows/github_pages.yml (2)
39-41: Consider caching npm packages more effectivelyThe current setup could be optimized for better CI performance.
- - run: npm install -g npm@10 - - run: npm ci + - run: npm install -g npm@10 + - name: Cache node_modules + uses: actions/cache@v4 + with: + path: node_modules + key: ${{ runner.os }}-node-${{ hashFiles('**/package-lock.json') }} + restore-keys: | + ${{ runner.os }}-node- + - run: npm ci
16-18: Consider enabling concurrent deployment cancellationFor faster deployments, consider enabling cancellation of in-progress deployments.
concurrency: group: "pages" - cancel-in-progress: false + cancel-in-progress: trueREADME.md (2)
58-66: Consider adding code examples for new featuresThe "What's New in v2.0?" section lists new features but would benefit from quick code snippets demonstrating their usage.
Consider adding examples like:
// Example: Setting point colors const colors = new Float32Array([/* RGB values */]); graph.setPointColors(colors); // Example: Using the new clustering feature const clusters = new Float32Array([/* cluster indices */]); graph.setPointClusters(clusters);
101-101: Use proper heading for License sectionReplace bold text with proper markdown heading.
-**MIT** +## MIT🧰 Tools
🪛 Markdownlint (0.37.0)
101-101: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.github/SECURITY.md(1 hunks).github/workflows/github_pages.yml(1 hunks).gitignore(1 hunks)CITATION.cff(1 hunks)LICENCE(1 hunks)README.md(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- LICENCE
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~50-~50: Loose punctuation mark.
Context: ...ph.render(); ``` - pointPositions: A Float32Array of `[x1, y1, x2, y2, ......
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~51-~51: Loose punctuation mark.
Context: ...y1, x2, y2, ..., xN, yN]. - **links**: A Float32Array of [sourceIndex1, targe...
(UNLIKELY_OPENING_PUNCTUATION)
.github/SECURITY.md
[style] ~9-~9: Consider using a different verb for a more formal wording.
Context: ... This gives us time to work with you to fix the issue before public exposure, reduc...
(FIX_RESOLVE)
🪛 Markdownlint (0.37.0)
README.md
101-101: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (1)
CITATION.cff (1)
23-23: Verify license change implications
The license has been changed from CC-BY-NC-4.0 to MIT. This is a significant change that makes the software more permissive.
✅ Verification successful
Let me run one more verification to check for any potential license headers in source files and look for the old CC-BY-NC-4.0 license references.
License change is consistent across the codebase
The license change from CC-BY-NC-4.0 to MIT is properly reflected across the entire codebase:
- The main license is declared as MIT in CITATION.cff, package.json, and LICENCE file
- No references to the old CC-BY-NC-4.0 license were found
- No conflicting license headers were found in source files
- All license declarations are consistent with the MIT license
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check license consistency across the project
echo "Checking license references..."
rg -g '!{node_modules,dist}/*' -i 'license|licence'
Length of output: 8745
Script:
#!/bin/bash
# Check for CC-BY-NC-4.0 references and license headers in source files
echo "Checking for CC-BY-NC-4.0 references..."
rg -i "CC-BY-NC-4.0|Creative Commons|Attribution-NonCommercial"
echo -e "\nChecking for license headers in source files..."
rg -g "*.{js,ts,jsx,tsx}" -i "^/\*\*?.*license" -A 3
Length of output: 320
| # Security Policy | ||
|
|
||
| ## Supported Versions | ||
|
|
||
| Security updates are applied only to the latest release. | ||
|
|
||
| ## Reporting a Vulnerability | ||
|
|
||
| If you have discovered a security vulnerability in this project, please report it privately. **Do not disclose it as a public issue.** This gives us time to work with you to fix the issue before public exposure, reducing the chance that the exploit will be used before a patch is released. | ||
|
|
||
| Please disclose it at [security advisory](https://github.com/cosmograph-org/cosmos/security/advisories/new). | ||
|
|
||
| This project is maintained by a team of volunteers on a reasonable-effort basis. As such, please give us at least 90 days to work on a fix before public exposure. |
There was a problem hiding this comment.
💡 Codebase verification
Security features need to be enabled and configured
The security policy document exists but critical security features are not enabled:
- Secret scanning is not enabled
- Dependabot security updates are not enabled
- No Dependabot configuration exists for automated dependency updates
To align with GitHub's security best practices, please:
- Enable "Secret scanning" in repository settings
- Enable "Dependabot alerts" and "Dependabot security updates"
- Add a
.github/dependabot.ymlconfiguration file to automate dependency updates
🔗 Analysis chain
Verify repository security settings
Let's ensure the repository has the necessary security features enabled to support this policy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check repository security settings
gh api repos/cosmograph-org/cosmos | jq '{
private: .private,
has_security_policy: .security_and_analysis.secret_scanning.status,
has_dependency_alerts: .security_and_analysis.dependabot_security_updates.status
}'
# Check if dependabot is configured
if [ -f ".github/dependabot.yml" ]; then
echo "Dependabot configuration found"
cat ".github/dependabot.yml"
else
echo "No dependabot configuration found"
fi
Length of output: 421
🧰 Tools
🪛 LanguageTool
[style] ~9-~9: Consider using a different verb for a more formal wording.
Context: ... This gives us time to work with you to fix the issue before public exposure, reduc...
(FIX_RESOLVE)
| ## Reporting a Vulnerability | ||
|
|
||
| If you have discovered a security vulnerability in this project, please report it privately. **Do not disclose it as a public issue.** This gives us time to work with you to fix the issue before public exposure, reducing the chance that the exploit will be used before a patch is released. | ||
|
|
||
| Please disclose it at [security advisory](https://github.com/cosmograph-org/cosmos/security/advisories/new). |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance vulnerability reporting guidelines
The reporting process could be more formal and comprehensive:
## Reporting a Vulnerability
-If you have discovered a security vulnerability in this project, please report it privately. **Do not disclose it as a public issue.** This gives us time to work with you to fix the issue before public exposure, reducing the chance that the exploit will be used before a patch is released.
+If you discover a security vulnerability in this project, please follow these steps:
+
+1. **Do not** disclose it as a public issue
+2. Submit a private report through our [security advisory](https://github.com/cosmograph-org/cosmos/security/advisories/new)
+3. Include in your report:
+ - A brief description of the vulnerability
+ - Steps to reproduce the issue
+ - Potential impact
+ - Suggested fix (if any)
+
+This process allows us to address the vulnerability before public exposure, minimizing potential security risks.
-Please disclose it at [security advisory](https://github.com/cosmograph-org/cosmos/security/advisories/new).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## Reporting a Vulnerability | |
| If you have discovered a security vulnerability in this project, please report it privately. **Do not disclose it as a public issue.** This gives us time to work with you to fix the issue before public exposure, reducing the chance that the exploit will be used before a patch is released. | |
| Please disclose it at [security advisory](https://github.com/cosmograph-org/cosmos/security/advisories/new). | |
| ## Reporting a Vulnerability | |
| If you discover a security vulnerability in this project, please follow these steps: | |
| 1. **Do not** disclose it as a public issue | |
| 2. Submit a private report through our [security advisory](https://github.com/cosmograph-org/cosmos/security/advisories/new) | |
| 3. Include in your report: | |
| - A brief description of the vulnerability | |
| - Steps to reproduce the issue | |
| - Potential impact | |
| - Suggested fix (if any) | |
| This process allows us to address the vulnerability before public exposure, minimizing potential security risks. |
🧰 Tools
🪛 LanguageTool
[style] ~9-~9: Consider using a different verb for a more formal wording.
Context: ... This gives us time to work with you to fix the issue before public exposure, reduc...
(FIX_RESOLVE)
| version: 2.0.0 | ||
| date-released: 2024-12-20 |
There was a problem hiding this comment.
💡 Codebase verification
Version mismatch and future release date need attention
The package.json shows version "2.0.0-alpha.8" while CITATION.cff declares "2.0.0". Additionally, the release date is set to December 2024, which is in the future.
- Update CITATION.cff version to match package.json's "2.0.0-alpha.8"
- Set the release date to the current date or when this alpha version was released
🔗 Analysis chain
Review the release date
The release date is set to December 2024, which is in the future. Consider updating it to the actual release date.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify version consistency across the project
echo "Checking version numbers in package.json and other files..."
rg -g '!{node_modules,dist}/*' '"version":\s*"2.0.0'
Length of output: 1394
rokotyan
temporarily deployed
to
github-pages
GitHub Actions
Inactive
rokotyan
temporarily deployed
to
github-pages
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (12)
src/stories/2. configuration.mdx (3)
33-33: Enhance link visibility distance range description.The description could be clearer about how the opacity transitions between the min and max distances.
Consider adding:
-The link will be fully opaque when its length is less than the first number in the array, and will have `linkVisibilityMinTransparency` transparency when its length is greater than the second number in the array. +The link's opacity transitions smoothly from fully opaque (when its length is less than the first number) to `linkVisibilityMinTransparency` (when its length exceeds the second number). For lengths between these values, opacity scales linearly.
35-35: Expand quadtree compatibility warning.The warning about GPU compatibility should provide more context about ANGLE and browser settings.
Consider adding:
-⚠ The algorithm might not work on some GPUs (e.g. Nvidia) and on Windows (unless you disable ANGLE in the browser settings). +⚠ The algorithm might not work on some GPUs (e.g. Nvidia) and on Windows due to ANGLE (Almost Native Graphics Layer Engine) limitations. To resolve this: +- Chrome: Visit chrome://flags/#use-angle and select "OpenGL" or disable ANGLE +- Firefox: Set gfx.angle.force-d3d11 to false in about:config +Note: Changing these settings may affect other WebGL applications.🧰 Tools
🪛 LanguageTool
[uncategorized] ~35-~35: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...applied only on component initialization and it can't be changed using the `setConfi...(COMMA_COMPOUND_SENTENCE)
48-48: Add performance implications for quadtree levels.The documentation should mention the performance trade-offs of different quadtree levels.
Consider adding:
-Barnes–Hut approximation depth. Can only be used when `useQuadtree` is set `true`. +Barnes–Hut approximation depth. Can only be used when `useQuadtree` is set `true`. Higher levels provide more accurate force calculations but require more computational resources. Lower levels improve performance at the cost of approximation accuracy.src/stories/3. api-reference.mdx (4)
296-296: Fix duplicate word.There's a duplicate occurrence of the word "method".
-This method method is used to convert +This method is used to convert🧰 Tools
🪛 LanguageTool
[duplication] ~296-~296: Possible typo: you repeated a word
Context: ...nPosition(coordinates) This method method is used to convert X and Y point coordi...(ENGLISH_WORD_REPEAT_RULE)
303-303: Fix missing word at sentence start.The sentence starts with a backtick instead of the word "This".
-`method converts X and Y point coordinates +This method converts X and Y point coordinates
324-328: Add memory usage warning for point tracking.The documentation should warn about potential memory implications of tracking many points.
Consider adding:
This method allows you to monitor the coordinates of specific points over time. This is particularly useful for observing dynamic changes in the positions of points during the simulation. The positions of the tracked points are updated on each tick of the Cosmos simulation. +⚠ **Note:** Tracking a large number of points can impact memory usage and performance, as position data is stored for each tracked point. Consider tracking only the essential points needed for your application. + * **`indices`** (Array): An array of numbers representing the indices of points to be tracked.
1-413: Add TypeScript type information to method signatures.The API documentation would benefit from including TypeScript type information for better developer experience.
Consider adding type information to method signatures. For example:
interface Point { x: number; y: number; } interface CosmosGraph { setPointPositions(positions: Float32Array): void; setPointColors(colors: Float32Array): void; getPointsInRange(selection: [[number, number], [number, number]]): Float32Array; // ... other methods }Would you like me to generate TypeScript interfaces for all the methods?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~89-~89: Loose punctuation mark.
Context: ...link. Here’s a breakdown: *r: Red value (0.0-1.0) *g: Gree...(UNLIKELY_OPENING_PUNCTUATION)
[duplication] ~296-~296: Possible typo: you repeated a word
Context: ...nPosition(coordinates) This method method is used to convert X and Y point coordi...(ENGLISH_WORD_REPEAT_RULE)
[style] ~333-~333: ‘in conjunction with’ might be wordy. Consider a shorter alternative.
Context: ...uring the simulation. Using this method in conjunction with the [trackPointPositionsByIndices](...(EN_WORDINESS_PREMIUM_IN_CONJUNCTION_WITH)
package.json (1)
95-98: Enhance ESLint configurationThe current ESLint configuration only extends Storybook recommendations. Consider enhancing it with:
- TypeScript-specific rules
- Project-specific conventions
- Integration with existing lint rules
Consider extending the configuration:
"eslintConfig": { "extends": [ + "eslint:recommended", + "plugin:@typescript-eslint/recommended", "plugin:storybook/recommended" - ] + ], + "rules": { + // Add project-specific rules here + } }src/stories/1. welcome.mdx (4)
1-3: Consider updating the title to reflect the version.Since this is for Cosmos 2.0, consider making the version explicit in the title.
-<Meta title="Welcome to Cosmos" /> +<Meta title="Welcome to Cosmos 2.0" />
7-7: Consider moving inline styles to a stylesheet.Instead of using inline styles, consider moving them to a separate CSS file for better maintainability.
-<p style={{ fontSize: '1.2rem' }}>This page contains examples of how to use Cosmos — a low-level library for visualizing network graphs and machine learning embeddings.</p> +<p className="cosmos-description">This page contains examples of how to use Cosmos — a low-level library for visualizing network graphs and machine learning embeddings.</p>
9-10: Consider self-hosting the video and moving styles to stylesheet.
- The video is currently hosted on GitHub which might have bandwidth limitations.
- Inline styles should be moved to a stylesheet.
Additionally, consider adding a fallback content inside the video tag for browsers that don't support video playback.
-<video style={{ width: '100%' }} src="https://user-images.githubusercontent.com/755708/173392407-9b05cbb6-d39e-4c2c-ab41-50900cfda823.mp4" loop autoPlay muted playsInline> +<video className="cosmos-demo-video" src="assets/videos/cosmos-demo.mp4" loop autoPlay muted playsInline> + Your browser does not support the video tag. You can <a href="assets/videos/cosmos-demo.mp4">download the video</a> instead. </video>
28-33: Enhance the configuration example with more comprehensive options and types.Consider:
- Adding TypeScript types for better documentation
- Including more essential configuration options
- Adding comments explaining each option
const config = { simulationRepulsion: 0.5, renderLinks: true, onClick: pointIndex => { console.log('Clicked point index: ', pointIndex) }, - /* ... */ + // Simulation parameters + simulationAlpha: 1, + simulationDecay: 0.02, + // Rendering options + pointColor: '#000000', + pointSize: 5, + linkColor: '#999999', + linkWidth: 1, + // Interaction handlers + onHover: (pointIndex: number | null) => void, + onDrag: (pointIndex: number) => void, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
logo.svgis excluded by!**/*.svgpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
.storybook/main.ts(1 hunks).storybook/manager.ts(1 hunks).storybook/preview.ts(1 hunks).storybook/style.css(1 hunks)package.json(3 hunks)src/stories/1. welcome.mdx(1 hunks)src/stories/2. configuration.mdx(1 hunks)src/stories/3. api-reference.mdx(1 hunks)src/stories/clusters.stories.ts(1 hunks)src/stories/experiments.stories.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .storybook/style.css
🚧 Files skipped from review as they are similar to previous changes (5)
- .storybook/manager.ts
- .storybook/main.ts
- .storybook/preview.ts
- src/stories/clusters.stories.ts
- src/stories/experiments.stories.ts
🧰 Additional context used
🪛 LanguageTool
src/stories/2. configuration.mdx
[uncategorized] ~10-~10: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...applied only on component initialization and it can't be changed using the `setConfi...
(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~27-~27: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...ue, links are rendered as curved lines. Otherwise as straight lines | false | | curvedL...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[style] ~31-~31: Consider shortening this phrase to just ‘whether’, unless you mean ‘regardless of whether’.
Context: ... default link arrow value that controls whether or not to display link arrows | false | | li...
(WHETHER)
[uncategorized] ~35-~35: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...applied only on component initialization and it can't be changed using the `setConfi...
(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~38-~38: You might be missing the article “the” here.
Context: ...false | ## Simulation configuration Cosmos layout algorithm was inspired by the [d...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
src/stories/3. api-reference.mdx
[uncategorized] ~89-~89: Loose punctuation mark.
Context: ...link. Here’s a breakdown: * r: Red value (0.0-1.0) * g: Gree...
(UNLIKELY_OPENING_PUNCTUATION)
[duplication] ~296-~296: Possible typo: you repeated a word
Context: ...nPosition(coordinates) This method method is used to convert X and Y point coordi...
(ENGLISH_WORD_REPEAT_RULE)
[style] ~333-~333: ‘in conjunction with’ might be wordy. Consider a shorter alternative.
Context: ...uring the simulation. Using this method in conjunction with the [trackPointPositionsByIndices](...
(EN_WORDINESS_PREMIUM_IN_CONJUNCTION_WITH)
🔇 Additional comments (4)
package.json (3)
3-3: Major changes in licensing and versioning
Referring to the previous review comment about version bump and license changes.
Also applies to: 8-8
13-13: Clarify build system strategy
The presence of both Rollup and Vite build scripts raises concerns:
- Potential confusion about which build system to use
- Increased maintenance overhead
- Possible inconsistencies between builds
Let's check if there are any differences between the build outputs:
Consider:
- Documenting the purpose of each build system
- Creating a migration plan if Vite is intended to replace Rollup
- Adding comments in the scripts to clarify when to use each build command
Also applies to: 16-18
✅ Verification successful
Migration from Rollup to Vite in progress
Based on the configurations examined, this appears to be an intentional migration from Rollup to Vite:
-
Both build systems produce similar outputs:
- ES and UMD formats
- Source maps enabled
- Same entry point (src/index.ts)
- Similar alias configurations
- Identical GLSL handling
-
Vite configuration includes improvements:
- Built-in TypeScript declaration generation (vite-plugin-dts)
- More structured external dependency handling
- Cleaner configuration syntax
The parallel existence is likely temporary during the migration phase, with Vite being the target build system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare build configurations and outputs
# Check for build configuration files
echo "Checking build configurations:"
fd -e config.js -e config.ts "rollup|vite"
# Check for documentation about build systems
echo "Checking for build documentation:"
rg -l "build|vite|rollup" "README.md" "docs/"
Length of output: 314
Script:
#!/bin/bash
# Let's examine the build configurations and any migration hints
# Check rollup config content
echo "=== Rollup Config ==="
cat rollup.config.js
# Check vite config content
echo -e "\n=== Vite Config ==="
cat vite.config.ts
# Look for comments or documentation about build systems in package.json
echo -e "\n=== Package.json Build Scripts ==="
cat package.json | jq -r '.scripts'
# Search for build-related comments in recent commits
echo -e "\n=== Recent Build-Related Commits ==="
git log --grep="build\|vite\|rollup" --max-count=5 --oneline
Length of output: 3755
34-75: Review dependency versions and compatibility
The addition of numerous new dependencies requires careful consideration:
- Storybook dependencies (v8.4.5) are using a very recent version
- Multiple d3-related packages and their type definitions
- Build tool dependencies (vite, typescript, etc.)
Let's check for potential issues:
Consider:
- Documenting the minimum supported Node.js version requirements
- Adding a dependency management strategy to handle future updates
- Setting up automated dependency update checks
✅ Verification successful
Let me gather more information about the Storybook dependencies to ensure version compatibility.
Dependencies are well-aligned with consistent versioning
The dependency analysis shows:
- All Storybook dependencies are consistently versioned at ^8.4.5
- D3 type definitions (@types/d3-*) align with their corresponding package versions (all at major version 3)
- TypeScript 5.5.2 is compatible with Storybook 8.x
- Node.js requirement (>=12.2.0) is sufficient for all dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify dependency compatibility and security
# Check for peer dependency warnings in package-lock.json
echo "Checking peer dependencies:"
fd -t f "package-lock.json" -x jq -r '.packages | .[] | select(.peerDependencies != null) | .name'
# Check if d3 package versions align with their @types versions
echo "Checking d3 package alignment:"
rg '"d3-[^"]+": "\^[0-9]+\.[0-9]+\.[0-9]+"' package.json
Length of output: 943
Script:
#!/bin/bash
# Check for Storybook dependencies and their versions
echo "Checking Storybook dependencies:"
rg '"@storybook/[^"]+": "\^[0-9]+\.[0-9]+\.[0-9]+"' package.json
# Check if there are any direct d3 dependencies that might conflict
echo "Checking direct d3 dependencies:"
rg '"d3": "' package.json
# Check Node.js version requirements
echo "Checking Node.js version:"
rg '"node":' package.json
rg '"engines":' package.json
# Check TypeScript version compatibility with Storybook
echo "Checking TypeScript version:"
rg '"typescript":' package.json
Length of output: 763
src/stories/1. welcome.mdx (1)
44-46: Expand examples section and ensure link stability.
Consider:
- Adding more examples covering common use cases
- Including embedded examples directly in the documentation
- Adding version information for the external package
src/stories/1. welcome.mdx
Outdated
| graph.render() | ||
| ``` | ||
|
|
||
| Where `pointPositions` - a Float32Array of `[x1, y1, x2, y2, x3, y3]` and `links` a Float32Array of `[sourceIndex1, targetIndex1, sourceIndex2, targetIndex2, sourceIndex3, targetIndex3]`. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance data format documentation with visual examples.
The current explanation of data formats could be clearer. Consider:
- Adding a visual diagram showing how points and links are structured
- Including a complete example with actual data
- Adding TypeScript interfaces for the data structures
-Where `pointPositions` - a Float32Array of `[x1, y1, x2, y2, x3, y3]` and `links` a Float32Array of `[sourceIndex1, targetIndex1, sourceIndex2, targetIndex2, sourceIndex3, targetIndex3]`.
+```typescript
+// Data structure examples:
+
+// Points: [x1, y1, x2, y2, x3, y3]
+const pointPositions = new Float32Array([
+ 0.0, 0.0, // Point 1 at (0,0)
+ 1.0, 0.0, // Point 2 at (1,0)
+ 0.5, 1.0, // Point 3 at (0.5,1)
+]);
+
+// Links: [sourceIndex1, targetIndex1, sourceIndex2, targetIndex2]
+const links = new Float32Array([
+ 0, 1, // Link from point 0 to point 1
+ 1, 2, // Link from point 1 to point 2
+ 2, 0, // Link from point 2 to point 0
+]);
+```
rokotyan
temporarily deployed
to
github-pages
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
src/stories/clusters.stories.ts (1)
29-51: Consider adding descriptions to the stories.While the story implementations look good, adding descriptions would improve documentation and help users understand the purpose of each clustering example.
export const Worm = WormStory as Story Worm.parameters = { + docs: { + description: 'Demonstrates worm-like cluster behavior where points form chain-like structures.' + }, sourceCode: [ { name: 'Story', code: wormStory }, ...sourceCodeAddonParams, ], } export const Radial = RadialStory as Story Radial.parameters = { + docs: { + description: 'Shows radial clustering where points organize in circular patterns.' + }, sourceCode: [ { name: 'Story', code: radialStory }, ...sourceCodeAddonParams, ], } export const WithLabels = WithLabelsStory as Story WithLabels.parameters = { + docs: { + description: 'Demonstrates clusters with labeled points for better identification.' + }, sourceCode: [ { name: 'Story', code: withLabelsStory }, ...sourceCodeAddonParams, ], }src/stories/3. api-reference.mdx (2)
296-296: Fix duplicate word "method".There's a duplicate word in the sentence.
-This method method is used to convert X and Y point coordinates +This method is used to convert X and Y point coordinates🧰 Tools
🪛 LanguageTool
[duplication] ~296-~296: Possible typo: you repeated a word
Context: ...nPosition(coordinates) This method method is used to convert X and Y point coordi...(ENGLISH_WORD_REPEAT_RULE)
89-92: Fix list formatting inconsistency.The RGBA parameters list has inconsistent indentation and punctuation.
- * **`r`**: Red value (0.0-1.0) - * **`g`**: Green value (0.0-1.0) - * **`b`**: Blue value (0.0-1.0) - * **`a`**: Alpha (opacity) value (0.0-1.0), where 0 is fully transparent and 1 is fully opaque. +* **`r`**: Red value (0.0-1.0) +* **`g`**: Green value (0.0-1.0) +* **`b`**: Blue value (0.0-1.0) +* **`a`**: Alpha (opacity) value (0.0-1.0), where 0 is fully transparent and 1 is fully opaque🧰 Tools
🪛 LanguageTool
[uncategorized] ~89-~89: Loose punctuation mark.
Context: ...link. Here’s a breakdown: *r: Red value (0.0-1.0) *g: Gree...(UNLIKELY_OPENING_PUNCTUATION)
src/stories/2. configuration.mdx (3)
8-35: Documentation improvements needed for configuration properties
- Color property documentation should be consistent. Some properties mention RGBA format while others don't.
- Numeric properties should include validation ranges where applicable.
- Consider adding code examples for complex properties.
Consider updating the documentation with these improvements:
| pointColor | The default color to use for points when no point colors are provided, or if the color value in the array is `undefined` or `null`. This can be either a hex color string (e.g., '#b3b3b3') or an array of RGBA values in the format `[red, green, blue, alpha]` where each value is a number between 0 and 255 | `#b3b3b3` | -| hoveredPointRingColor | Hovered point ring color hex value or an array of RGBA values | `white` | +| hoveredPointRingColor | Hovered point ring color. This can be either a hex color string (e.g., '#ffffff') or an array of RGBA values in the format `[red, green, blue, alpha]` where each value is a number between 0 and 255 | `white` | | spaceSize | Simulation space size (max 8192) | `8192` | +| pointSize | The default size value to use for points when no point sizes are provided or if the size value in the array is `undefined` or `null`. Valid range: 1-100 | `4` |Also consider adding usage examples:
// Example configurations const config = { // Basic setup pointColor: '#ff0000', // Red points pointSize: 6, // Larger than default // Using RGBA colors linkColor: [255, 0, 0, 128], // Semi-transparent red // Complex setup linkVisibilityDistanceRange: [100, 300], // Longer fade distance }🧰 Tools
🪛 LanguageTool
[uncategorized] ~10-~10: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...applied only on component initialization and it can't be changed using the `setConfi...(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~27-~27: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...ue, links are rendered as curved lines. Otherwise as straight lines |false| | curvedL...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[style] ~31-~31: Consider shortening this phrase to just ‘whether’, unless you mean ‘regardless of whether’.
Context: ... default link arrow value that controls whether or not to display link arrows |false| | li...(WHETHER)
[uncategorized] ~35-~35: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...applied only on component initialization and it can't be changed using the `setConfi...(COMMA_COMPOUND_SENTENCE)
37-54: Consider enhancing technical documentationThe simulation configuration section would benefit from:
- More detailed explanations of technical concepts
- Visual examples showing the impact of different parameters
- Performance implications for each parameter
Consider adding a new section with visual examples and performance implications:
## Parameter Effects and Performance ### Visual Examples - Add diagrams or animations showing the effect of different `simulationDecay` values - Show side-by-side comparisons of different `simulationRepulsionTheta` settings ### Performance Considerations - `simulationRepulsionQuadtreeLevels`: Higher values provide more accurate force calculations but impact performance - `useQuadtree`: Enables performance optimization but may not work on all hardware
10-10: Grammar and style improvements neededSeveral minor grammatical issues need attention:
- Add commas in compound sentences
- Improve clarity of conjunctive adverbs
- Simplify redundant phrases
Apply these corrections:
-| disableSimulation | Do not run the simulation, just render the graph. Cosmos uses the `x` and `y` values of the points' data to determine their position in the graph. If `x` and `y` values are not specified, the position of the points will be assigned randomly. This property will be applied only on component initialization and it can't be changed using the `setConfig` method | `false` | +| disableSimulation | Do not run the simulation, just render the graph. Cosmos uses the `x` and `y` values of the points' data to determine their position in the graph. If `x` and `y` values are not specified, the position of the points will be assigned randomly. This property will be applied only on component initialization, and it can't be changed using the `setConfig` method | `false` | -| curvedLinks | If set to true, links are rendered as curved lines. Otherwise as straight lines | `false` | +| curvedLinks | If set to true, links are rendered as curved lines. Otherwise, as straight lines | `false` | -| linkArrows | The default link arrow value that controls whether or not to display link arrows | `false` | +| linkArrows | The default link arrow value that controls whether to display link arrows | `false` |Also applies to: 27-27, 31-31, 35-35
🧰 Tools
🪛 LanguageTool
[uncategorized] ~10-~10: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...applied only on component initialization and it can't be changed using the `setConfi...(COMMA_COMPOUND_SENTENCE)
package.json (2)
13-18: Consider consolidating build systemsThe project now maintains two parallel build systems:
- Rollup (
buildscript)- Vite (
build:vitescript)This could lead to:
- Maintenance overhead
- Potential inconsistencies in output
- Confusion about which build system to use
Consider:
- Documenting the purpose of each build system
- Migrating completely to Vite if Rollup is no longer needed
95-98: Consider enhancing ESLint configurationWhile the Storybook ESLint plugin is correctly configured, consider extending the configuration to include:
- TypeScript-specific rules
- Import ordering rules
- Other existing ESLint configurations from the project
Example enhancement:
"eslintConfig": { "extends": [ + "eslint:recommended", + "plugin:@typescript-eslint/recommended", "plugin:storybook/recommended" ] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
logo.svgis excluded by!**/*.svgpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
.storybook/main.ts(1 hunks).storybook/manager-head.html(1 hunks).storybook/manager.ts(1 hunks).storybook/preview.ts(1 hunks).storybook/style.css(1 hunks)package.json(3 hunks)src/stories/1. welcome.mdx(1 hunks)src/stories/2. configuration.mdx(1 hunks)src/stories/3. api-reference.mdx(1 hunks)src/stories/clusters.stories.ts(1 hunks)src/stories/experiments.stories.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .storybook/manager-head.html
🚧 Files skipped from review as they are similar to previous changes (6)
- .storybook/style.css
- .storybook/manager.ts
- .storybook/main.ts
- .storybook/preview.ts
- src/stories/1. welcome.mdx
- src/stories/experiments.stories.ts
🧰 Additional context used
🪛 LanguageTool
src/stories/3. api-reference.mdx
[uncategorized] ~89-~89: Loose punctuation mark.
Context: ...link. Here’s a breakdown: * r: Red value (0.0-1.0) * g: Gree...
(UNLIKELY_OPENING_PUNCTUATION)
[duplication] ~296-~296: Possible typo: you repeated a word
Context: ...nPosition(coordinates) This method method is used to convert X and Y point coordi...
(ENGLISH_WORD_REPEAT_RULE)
[style] ~333-~333: ‘in conjunction with’ might be wordy. Consider a shorter alternative.
Context: ...uring the simulation. Using this method in conjunction with the [trackPointPositionsByIndices](...
(EN_WORDINESS_PREMIUM_IN_CONJUNCTION_WITH)
src/stories/2. configuration.mdx
[uncategorized] ~10-~10: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...applied only on component initialization and it can't be changed using the `setConfi...
(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~27-~27: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...ue, links are rendered as curved lines. Otherwise as straight lines | false | | curvedL...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[style] ~31-~31: Consider shortening this phrase to just ‘whether’, unless you mean ‘regardless of whether’.
Context: ... default link arrow value that controls whether or not to display link arrows | false | | li...
(WHETHER)
[uncategorized] ~35-~35: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...applied only on component initialization and it can't be changed using the `setConfi...
(COMMA_COMPOUND_SENTENCE)
🔇 Additional comments (8)
src/stories/clusters.stories.ts (2)
1-5: LGTM! Clean and well-organized imports.
The imports are properly structured, with clear separation between Storybook types and story components.
13-20: LGTM! Well-configured Storybook metadata.
The metadata is properly typed with Meta<CosmosStoryProps> and includes appropriate configuration to disable controls, which is suitable for example stories.
src/stories/3. api-reference.mdx (1)
1-413: LGTM! Comprehensive and well-structured API documentation.
The API reference is thorough and well-organized, with clear descriptions, parameter explanations, and practical examples for each method. The documentation will be valuable for users implementing the Cosmos graph visualization library.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~89-~89: Loose punctuation mark.
Context: ...link. Here’s a breakdown: * r: Red value (0.0-1.0) * g: Gree...
(UNLIKELY_OPENING_PUNCTUATION)
[duplication] ~296-~296: Possible typo: you repeated a word
Context: ...nPosition(coordinates) This method method is used to convert X and Y point coordi...
(ENGLISH_WORD_REPEAT_RULE)
[style] ~333-~333: ‘in conjunction with’ might be wordy. Consider a shorter alternative.
Context: ...uring the simulation. Using this method in conjunction with the [trackPointPositionsByIndices](...
(EN_WORDINESS_PREMIUM_IN_CONJUNCTION_WITH)
src/stories/2. configuration.mdx (1)
1-4: LGTM! Follows Storybook MDX best practices.
package.json (4)
3-3: Major changes in licensing and versioning
- Version bump to 2.0.0-alpha.8 indicates breaking changes
- License change from CC-BY-NC-4.0 to MIT is significant:
- MIT is more permissive
- Allows commercial use
- May affect existing users
Please ensure:
- All contributors have agreed to the license change
- Breaking changes are documented in CHANGELOG.md
- Migration guide is provided for users
Also applies to: 8-8
Line range hint 78-89: Dependencies look well-maintained
Good job maintaining consistent versions across d3 packages. The dependencies section is well-organized with appropriate version constraints.
72-72:
Consider using a stable TypeScript version
TypeScript ^5.5.2 is currently in beta. Consider using the latest stable version (5.3.x) to avoid potential issues.
69-71: Verify build tool dependencies compatibility
The project includes multiple build tool-related dependencies:
- rollup and plugins
- vite and plugins
- Both include GLSL plugins (
rollup-plugin-glslifyandvite-plugin-glsl)
Consider:
- Ensuring compatibility between these tools
- Removing unused build tool dependencies if migrating to Vite
Also applies to: 73-75
✅ Verification successful
Both Rollup and Vite configurations are actively used with GLSL files
The codebase actively uses both build tools with their respective GLSL plugins:
- Multiple
.fragand.vertshader files are present and imported throughout the modules rollup.config.jsusesrollup-plugin-glslifyfor shader compilationvite.config.tsusesvite-plugin-glslfor shader compilation
Both build tool setups appear to be in active use, suggesting a transition period or dual build system setup. Keep both GLSL plugins as they are currently needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for GLSL file usage to determine which plugin is needed
rg -t glsl '' .
Length of output: 46
Script:
#!/bin/bash
# Search for GLSL file extensions and shader files
fd -e glsl -e frag -e vert .
# Check for imports or usage of GLSL files
rg -g '*.{js,ts,jsx,tsx}' '\.(?:glsl|frag|vert)'
# Check build configuration files for usage of these plugins
rg -g '{vite,rollup}*.{js,ts,cjs,mjs}' 'glsl|rollup-plugin-glslify|vite-plugin-glsl'
# Check if there are any Vite config files
fd 'vite.config'
Length of output: 6201
rokotyan
temporarily deployed
to
github-pages
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (14)
src/stories/2. configuration.mdx (8)
12-12: Add validation range for spaceSize propertyThe description should specify the minimum allowed value for
spaceSizesince only the maximum (8192) is mentioned.
15-17: Add validation ranges for point size propertiesThe
pointSizeandpointSizeScaleproperties should include their valid numerical ranges in their descriptions to prevent invalid inputs.
25-26: Add validation ranges for link width propertiesThe
linkWidthandlinkWidthScaleproperties should include their valid numerical ranges in their descriptions to prevent invalid inputs.
28-30: Clarify curved link parametersThe curved link parameters need more detailed explanations:
curvedLinkSegments: Specify the valid range and performance implicationscurvedLinkWeight: Explain how different values affect the curve shapecurvedLinkControlPointDistance: Add a visual example or diagram
43-46: Enhance simulation force coefficient descriptionsConsider adding:
- Visual examples or diagrams showing the effects of different values
- Cross-references between related properties (e.g., how
simulationGravityinteracts withsimulationRepulsion)- Links to relevant physics concepts for better understanding
47-48: Clarify quadtree-related propertiesThe
simulationRepulsionThetaandsimulationRepulsionQuadtreeLevelsproperties need more context:
- Explain performance implications of different values
- Add a note about memory usage considerations
- Consider adding a link to a detailed explanation of the Barnes-Hut algorithm
51-51: Fix array formatting in tableThe array in
simulationLinkDistRandomVariationRangebreaks the table formatting. Consider using a different format or line breaks to maintain table readability.
10-10: Fix grammar and punctuation issuesPlease apply the following corrections:
- Line 10: Add a comma before "and" in "initialization and it can't"
- Line 27: Add a comma after "Otherwise"
- Line 31: Consider changing "whether or not" to simply "whether"
- Line 35: Add a comma before "and" in "initialization and it can't"
- Line 39: Add "The" before "Cosmos layout algorithm"
Also applies to: 27-27, 31-31, 35-35, 39-39
🧰 Tools
🪛 LanguageTool
[uncategorized] ~10-~10: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...applied only on component initialization and it can't be changed using the `setConfi...(COMMA_COMPOUND_SENTENCE)
src/stories/3. api-reference.mdx (5)
296-296: Fix duplicate word in documentation.Remove the duplicate occurrence of "method".
-This method method is used to convert X and Y point coordinates +This method is used to convert X and Y point coordinates🧰 Tools
🪛 LanguageTool
[duplication] ~296-~296: Possible typo: you repeated a word
Context: ...nPosition(coordinates) This method method is used to convert X and Y point coordi...(ENGLISH_WORD_REPEAT_RULE)
87-93: Standardize bullet point formatting.The indentation and formatting of bullet points is inconsistent with other sections. Consider standardizing the format.
- * **`r`**: Red value (0.0-1.0) - * **`g`**: Green value (0.0-1.0) - * **`b`**: Blue value (0.0-1.0) - * **`a`**: Alpha (opacity) value (0.0-1.0) +* **`r`**: Red value (0.0-1.0) +* **`g`**: Green value (0.0-1.0) +* **`b`**: Blue value (0.0-1.0) +* **`a`**: Alpha (opacity) value (0.0-1.0)🧰 Tools
🪛 LanguageTool
[uncategorized] ~89-~89: Loose punctuation mark.
Context: ...link. Here’s a breakdown: *r: Red value (0.0-1.0) *g: Gree...(UNLIKELY_OPENING_PUNCTUATION)
5-9: Enhance setConfig documentation with technical details.Consider adding information about:
- Whether the method triggers an immediate re-render
- Performance implications of changing configuration during runtime
- Whether partial configuration updates are supported
322-329: Add performance considerations to tracking methods.The documentation should include:
- Performance impact of tracking multiple points
- Memory usage considerations
- Recommended maximum number of points to track
1-413: Enhance documentation with additional developer guidance.While the API documentation is comprehensive, consider adding:
- Cross-references between related methods (e.g., linking selection methods together)
- Common usage patterns and best practices
- Performance optimization tips
- Troubleshooting guide for common issues
🧰 Tools
🪛 LanguageTool
[uncategorized] ~89-~89: Loose punctuation mark.
Context: ...link. Here’s a breakdown: *r: Red value (0.0-1.0) *g: Gree...(UNLIKELY_OPENING_PUNCTUATION)
[duplication] ~296-~296: Possible typo: you repeated a word
Context: ...nPosition(coordinates) This method method is used to convert X and Y point coordi...(ENGLISH_WORD_REPEAT_RULE)
[style] ~333-~333: ‘in conjunction with’ might be wordy. Consider a shorter alternative.
Context: ...uring the simulation. Using this method in conjunction with the [trackPointPositionsByIndices](...(EN_WORDINESS_PREMIUM_IN_CONJUNCTION_WITH)
package.json (1)
95-98: Integrate Storybook ESLint config with existing rulesThe current ESLint configuration only extends Storybook rules. Consider:
- Merging with existing ESLint rules
- Ensuring no conflicts with current linting setup
- Maintaining consistent code style across all files
"eslintConfig": { "extends": [ + "standard", + "plugin:@typescript-eslint/recommended", "plugin:storybook/recommended" ] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
logo.svgis excluded by!**/*.svgpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
.storybook/main.ts(1 hunks).storybook/manager-head.html(1 hunks).storybook/manager.ts(1 hunks).storybook/preview.ts(1 hunks).storybook/style.css(1 hunks)package.json(3 hunks)src/attribution.ts(1 hunks)src/stories/1. welcome.mdx(1 hunks)src/stories/2. configuration.mdx(1 hunks)src/stories/3. api-reference.mdx(1 hunks)src/stories/clusters.stories.ts(1 hunks)src/stories/experiments.stories.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- .storybook/style.css
- .storybook/manager-head.html
- src/attribution.ts
- .storybook/manager.ts
- src/stories/1. welcome.mdx
- .storybook/preview.ts
- .storybook/main.ts
- src/stories/experiments.stories.ts
- src/stories/clusters.stories.ts
🧰 Additional context used
🪛 LanguageTool
src/stories/2. configuration.mdx
[uncategorized] ~10-~10: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...applied only on component initialization and it can't be changed using the `setConfi...
(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~27-~27: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...ue, links are rendered as curved lines. Otherwise as straight lines | false | | curvedL...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[style] ~31-~31: Consider shortening this phrase to just ‘whether’, unless you mean ‘regardless of whether’.
Context: ... default link arrow value that controls whether or not to display link arrows | false | | li...
(WHETHER)
[uncategorized] ~35-~35: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...applied only on component initialization and it can't be changed using the `setConfi...
(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~38-~38: You might be missing the article “the” here.
Context: ...false | ## Simulation configuration Cosmos layout algorithm was inspired by the [d...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
src/stories/3. api-reference.mdx
[uncategorized] ~89-~89: Loose punctuation mark.
Context: ...link. Here’s a breakdown: * r: Red value (0.0-1.0) * g: Gree...
(UNLIKELY_OPENING_PUNCTUATION)
[duplication] ~296-~296: Possible typo: you repeated a word
Context: ...nPosition(coordinates) This method method is used to convert X and Y point coordi...
(ENGLISH_WORD_REPEAT_RULE)
[style] ~333-~333: ‘in conjunction with’ might be wordy. Consider a shorter alternative.
Context: ...uring the simulation. Using this method in conjunction with the [trackPointPositionsByIndices](...
(EN_WORDINESS_PREMIUM_IN_CONJUNCTION_WITH)
🔇 Additional comments (3)
src/stories/3. api-reference.mdx (1)
1-4: LGTM! Clean and standard MDX setup.
The file follows the standard Storybook MDX structure with proper imports and meta configuration.
package.json (2)
3-3: Major changes in licensing and versioning
- Version bump to 2.0.0-alpha.8 indicates breaking changes
- License change from CC-BY-NC-4.0 to MIT is significant:
- MIT is more permissive
- Allows commercial use
- May affect existing users
Please ensure:
- All contributors have agreed to the license change
- Breaking changes are documented in CHANGELOG.md
- Migration guide is provided for users
Also applies to: 8-8
13-13: Clarify build system strategy
The project now has two build systems (Rollup and Vite) running in parallel. This could lead to:
- Confusion about which build system to use
- Maintenance overhead
- Potential inconsistencies in builds
Consider:
- Documenting the purpose of each build system
- Planning for migration to a single build system
- Ensuring both systems produce identical outputs
Also applies to: 17-18
|
|
||
| ### <a name="get_point_positions" href="#get_point_positions">#</a> graph.<b>getPointPositions</b>() | ||
|
|
||
| This method retrieves the current X and Y coordinates of all points in the graph as an array of numbers. The returned array is in the same order as the point positions were added to the graph. | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document return types and error handling.
Several getter methods lack comprehensive documentation about:
- Return type specifications
- Error handling scenarios
- Return value when the operation fails
Consider enhancing the documentation for methods like getPointPositions() and getSelectedIndices().
Also applies to: 275-278
| "@jls-digital/storybook-addon-code": "^1.0.4", | ||
| "@rollup/plugin-alias": "^3.1.9", | ||
| "@rollup/plugin-commonjs": "^22.0.1", | ||
| "@rollup/plugin-node-resolve": "^13.3.0", | ||
| "@rollup/plugin-terser": "^0.4.4", | ||
| "@rollup/plugin-typescript": "^11.1.6", | ||
| "@storybook/addon-essentials": "^8.4.5", | ||
| "@storybook/addon-interactions": "^8.4.5", | ||
| "@storybook/blocks": "^8.4.5", | ||
| "@storybook/html": "^8.4.5", | ||
| "@storybook/html-vite": "^8.4.5", | ||
| "@storybook/test": "^8.4.5", | ||
| "@types/d3-array": "^3.0.3", | ||
| "@types/d3-color": "^3.1.0", | ||
| "@types/d3-drag": "^3.0.7", | ||
| "@types/d3-ease": "^3.0.0", | ||
| "@types/d3-scale": "^4.0.2", | ||
| "@types/d3-scale-chromatic": "^3.0.3", | ||
| "@types/d3-selection": "^3.0.2", | ||
| "@types/d3-transition": "^3.0.1", | ||
| "@types/d3-zoom": "^3.0.1", | ||
| "@typescript-eslint/eslint-plugin": "^5.30.5", | ||
| "@typescript-eslint/parser": "^5.30.5", | ||
| "@zerollup/ts-transform-paths": "^1.7.18", | ||
| "d3-scale-chromatic": "^3.1.0", | ||
| "eslint": "^8.19.0", | ||
| "eslint-config-standard": "^17.0.0", | ||
| "eslint-plugin-import": "^2.26.0", | ||
| "eslint-plugin-node": "^11.1.0", | ||
| "eslint-plugin-promise": "^6.0.0", | ||
| "eslint-plugin-storybook": "^0.11.1", | ||
| "eslint-plugin-unicorn": "^43.0.1", | ||
| "lint-staged": "^13.0.3", | ||
| "pre-commit": "^1.2.2", | ||
| "remark-gfm": "^4.0.0", | ||
| "rollup": "^2.76.0", | ||
| "rollup-plugin-glslify": "^1.3.0", | ||
| "rollup-plugin-typescript2": "^0.32.1", | ||
| "ttypescript": "^1.5.15", | ||
| "typescript": "^4.7.4" | ||
| "storybook": "^8.4.5", | ||
| "typescript": "^5.5.2", | ||
| "vite": "^5.4.11", | ||
| "vite-plugin-dts": "^4.3.0", | ||
| "vite-plugin-glsl": "^1.3.0" |
There was a problem hiding this comment.
💡 Codebase verification
Update TypeScript version and verify lock file inclusion
The TypeScript version (5.5.2) specified in package.json is a beta version, while the current stable version is 5.7.2. All Storybook dependencies are properly aligned at version 8.4.5. While package-lock.json exists in the repository, it should be included in the PR for review to ensure deterministic builds.
- Update
"typescript": "^5.5.2"to"typescript": "^5.7.2" - Include package-lock.json in the PR for review
🔗 Analysis chain
Review dependency versions and security implications
- TypeScript v5.5.2 is currently in beta. Consider using the latest stable version.
- Verify compatibility between Storybook v8.4.5 and its addons.
- Consider adding package-lock.json or yarn.lock to the PR for deterministic builds.
The previous script showed TypeScript 5.7.2 is the latest stable version. Let's verify the lock files and check version alignments before making the final response.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check latest stable versions
echo "Checking TypeScript latest stable version:"
curl -s https://registry.npmjs.org/typescript/latest | jq '.version'
# Check for potential security vulnerabilities
echo "Checking for known vulnerabilities:"
curl -s "https://api.snyk.io/api/v1/vuln/npm/@storybook%[email protected]" \
-H "Authorization: token $SNYK_TOKEN"
Length of output: 349
Script:
#!/bin/bash
# Let's check if package-lock.json or yarn.lock exists
echo "Checking for lock files:"
fd -e json -e lock "^(package-lock|yarn).*(json|lock)$"
# Let's check the actual TypeScript version in node_modules
echo -e "\nChecking for package.json TypeScript version:"
rg '"typescript"' package.json -A 1
# Let's verify Storybook versions alignment
echo -e "\nChecking Storybook related dependencies:"
rg '@storybook' package.json
Length of output: 687
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/index.ts (1)
802-808:⚠️ Potential issueReorder clear and destroy calls
The
reglInstance.clear()is called afterreglInstance.destroy(), which may not have the intended effect.Apply this diff to reorder the calls:
- this.reglInstance.destroy() - // Clears the canvas after particle system is destroyed - this.reglInstance.clear({ - color: this.store.backgroundColor, - depth: 1, - stencil: 0, - }) + // Clear the canvas before destroying + this.reglInstance.clear({ + color: this.store.backgroundColor, + depth: 1, + stencil: 0, + }) + this.reglInstance.destroy()
🧹 Nitpick comments (4)
src/modules/Zoom/index.ts (1)
139-141: Consider adding validation for negative space radiusThe
convertSpaceToScreenRadiusmethod should validate that the inputspaceRadiusis non-negative to prevent potential rendering issues.public convertSpaceToScreenRadius (spaceRadius: number): number { + if (spaceRadius < 0) { + console.warn('Space radius should be non-negative') + spaceRadius = Math.abs(spaceRadius) + } const { config: { scalePointsOnZoom }, store: { maxPointSize }, eventTransform: { k } } = this let size = spaceRadius * 2Also applies to: 141-146
src/index.ts (3)
78-86: Good improvement in canvas initializationThe canvas is now created programmatically and properly styled. However, consider adding error handling for cases where the div element is null.
public constructor (div: HTMLDivElement, config?: GraphConfigInterface) { + if (!div) { + throw new Error('Container div element is required') + } if (config) this.config.init(config) this.store.div = div
960-964: Consider adding a comment explaining the double drag callWhile the code includes a brief comment about preventing sudden jumps, it would be helpful to provide more detailed documentation about why two drag operations are necessary.
if (this.dragInstance.isActive) { - // To prevent the dragged point from suddenly jumping, run the drag function twice + // Execute drag twice to ensure smooth point movement: + // 1. First call updates the point's position based on mouse movement + // 2. Second call smooths out any potential discontinuities in the movement + // This prevents visual artifacts where points appear to "jump" during drag operations this.points.drag() this.points.drag() }
990-994: Improve type handling in updateMousePositionThe current implementation uses type assertions and optional chaining. Consider using type guards for cleaner type handling.
- private updateMousePosition (event: MouseEvent | D3DragEvent<HTMLCanvasElement, undefined, Hovered>): void { - if (!event) return - const mouseX = (event as MouseEvent).offsetX ?? (event as D3DragEvent<HTMLCanvasElement, undefined, Hovered>).x - const mouseY = (event as MouseEvent).offsetY ?? (event as D3DragEvent<HTMLCanvasElement, undefined, Hovered>).y + private updateMousePosition (event: MouseEvent | D3DragEvent<HTMLCanvasElement, undefined, Hovered>): void { + if (!event) return + + const isMouseEvent = (e: MouseEvent | D3DragEvent<HTMLCanvasElement, undefined, Hovered>): e is MouseEvent => + 'offsetX' in e && 'offsetY' in e + + const [mouseX, mouseY] = isMouseEvent(event) + ? [event.offsetX, event.offsetY] + : [event.x, event.y]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/index.ts(19 hunks)src/modules/Zoom/index.ts(4 hunks)
🔇 Additional comments (2)
src/modules/Zoom/index.ts (2)
67-75: Good addition of safeguards for equal extent values
The new logic handles cases where x or y extent values are equal by adjusting them by a small amount (0.5 pixels) to prevent division by zero in the scale calculations.
17-17: Verify event callback handling
The event callbacks have been simplified to use direct config properties. While this is cleaner, we should ensure all event handlers properly check for undefined callbacks before invocation.
Also applies to: 33-33, 39-39
✅ Verification successful
Based on the search results, I can now generate the final response:
Event callbacks are properly guarded against undefined values
The codebase consistently uses the optional chaining operator (?.) for both config properties and their callback invocations across all event handlers in both Zoom and Drag modules. This pattern safely handles undefined callbacks:
src/modules/Zoom/index.ts: All zoom callbacks (onZoomStart,onZoom,onZoomEnd)src/modules/Drag/index.ts: All drag callbacks (onDragStart,onDrag,onDragEnd)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other event callback patterns in the codebase
rg -A 2 "config\?\.\w+\?\." --type ts
Length of output: 1202
| private addAttribution (): void { | ||
| if (this.config.disableAttribution) return | ||
| this.attributionDivElement = document.createElement('div') | ||
| this.attributionDivElement.style.cssText = ` | ||
| cursor: pointer; | ||
| user-select: none; | ||
| position: absolute; | ||
| margin: 0 0.6rem 0.6rem 0; | ||
| line-height: 0; | ||
| bottom: 0; | ||
| right: 0; | ||
| color: var(--cosmos-attribution-color); | ||
| ` | ||
| this.attributionDivElement.onclick = (): void => window.open('https://cosmograph.app/', '_blank')?.focus() | ||
| const svgParser = new DOMParser() | ||
| const svgElement = svgParser.parseFromString(attributionSvg, 'image/svg+xml').firstChild as SVGElement | ||
| this.attributionDivElement.appendChild(svgElement) | ||
| this.store.div?.appendChild(this.attributionDivElement) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance attribution element security
The attribution implementation could benefit from additional security measures when handling SVG content.
private addAttribution (): void {
if (this.config.disableAttribution) return
this.attributionDivElement = document.createElement('div')
+ // Sanitize styles to prevent CSS injection
+ const safeStyles = [
+ 'cursor: pointer',
+ 'user-select: none',
+ 'position: absolute',
+ 'margin: 0 0.6rem 0.6rem 0',
+ 'line-height: 0',
+ 'bottom: 0',
+ 'right: 0',
+ 'color: var(--cosmos-attribution-color)'
+ ].join(';')
- this.attributionDivElement.style.cssText = `
- cursor: pointer;
- user-select: none;
- position: absolute;
- margin: 0 0.6rem 0.6rem 0;
- line-height: 0;
- bottom: 0;
- right: 0;
- color: var(--cosmos-attribution-color);
- `
+ this.attributionDivElement.style.cssText = safeStyles
this.attributionDivElement.onclick = (): void => window.open('https://cosmograph.app/', '_blank')?.focus()
const svgParser = new DOMParser()
+ // Validate SVG content before parsing
+ if (!attributionSvg.startsWith('<svg') || !attributionSvg.endsWith('</svg>')) {
+ console.warn('Invalid SVG content in attribution')
+ return
+ }
const svgElement = svgParser.parseFromString(attributionSvg, 'image/svg+xml').firstChild as SVGElement
+ if (!(svgElement instanceof SVGElement)) {
+ console.warn('Failed to parse attribution SVG')
+ return
+ }
this.attributionDivElement.appendChild(svgElement)
this.store.div?.appendChild(this.attributionDivElement)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private addAttribution (): void { | |
| if (this.config.disableAttribution) return | |
| this.attributionDivElement = document.createElement('div') | |
| this.attributionDivElement.style.cssText = ` | |
| cursor: pointer; | |
| user-select: none; | |
| position: absolute; | |
| margin: 0 0.6rem 0.6rem 0; | |
| line-height: 0; | |
| bottom: 0; | |
| right: 0; | |
| color: var(--cosmos-attribution-color); | |
| ` | |
| this.attributionDivElement.onclick = (): void => window.open('https://cosmograph.app/', '_blank')?.focus() | |
| const svgParser = new DOMParser() | |
| const svgElement = svgParser.parseFromString(attributionSvg, 'image/svg+xml').firstChild as SVGElement | |
| this.attributionDivElement.appendChild(svgElement) | |
| this.store.div?.appendChild(this.attributionDivElement) | |
| private addAttribution (): void { | |
| if (this.config.disableAttribution) return | |
| this.attributionDivElement = document.createElement('div') | |
| // Sanitize styles to prevent CSS injection | |
| const safeStyles = [ | |
| 'cursor: pointer', | |
| 'user-select: none', | |
| 'position: absolute', | |
| 'margin: 0 0.6rem 0.6rem 0', | |
| 'line-height: 0', | |
| 'bottom: 0', | |
| 'right: 0', | |
| 'color: var(--cosmos-attribution-color)' | |
| ].join(';') | |
| this.attributionDivElement.style.cssText = safeStyles | |
| this.attributionDivElement.onclick = (): void => window.open('https://cosmograph.app/', '_blank')?.focus() | |
| const svgParser = new DOMParser() | |
| // Validate SVG content before parsing | |
| if (!attributionSvg.startsWith('<svg') || !attributionSvg.endsWith('</svg>')) { | |
| console.warn('Invalid SVG content in attribution') | |
| return | |
| } | |
| const svgElement = svgParser.parseFromString(attributionSvg, 'image/svg+xml').firstChild as SVGElement | |
| if (!(svgElement instanceof SVGElement)) { | |
| console.warn('Failed to parse attribution SVG') | |
| return | |
| } | |
| this.attributionDivElement.appendChild(svgElement) | |
| this.store.div?.appendChild(this.attributionDivElement) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (19)
src/modules/Clusters/index.ts (2)
194-199: Add error handling for command execution failuresThe run method should handle potential WebGL errors during command execution.
Consider adding error handling:
public run (): void { if (!this.data.pointClusters && !this.data.clusterPositions) return - this.clearCentermassCommand?.() - this.calculateCentermassCommand?.() - this.applyForcesCommand?.() + try { + this.clearCentermassCommand?.() + this.calculateCentermassCommand?.() + this.applyForcesCommand?.() + } catch (error) { + console.error('Failed to execute cluster simulation commands:', error) + throw error + } }
1-200: Consider texture size optimization for better performanceThe current implementation uses fixed texture sizes based on point and cluster counts. Consider implementing dynamic texture size adjustment based on the actual data requirements and GPU capabilities.
Recommendations:
- Add texture size constraints based on
gl.getParameter(gl.MAX_TEXTURE_SIZE)- Implement power-of-two texture dimensions for better compatibility
- Consider texture packing strategies to minimize GPU memory usage
README.md (1)
46-62: Consider adding TypeScript type annotations for better developer experience.While the examples are clear, adding TypeScript types would help developers understand the expected data structures better.
-const pointPositions = new Float32Array([ +const pointPositions: Float32Array = new Float32Array([ 0.0, 0.0, // Point 1 at (0,0) 1.0, 0.0, // Point 2 at (1,0) 0.5, 1.0, // Point 3 at (0.5,1) ]); -const links = new Float32Array([ +const links: Float32Array = new Float32Array([ 0, 1, // Link from point 0 to point 1 1, 2, // Link from point 1 to point 2 2, 0, // Link from point 2 to point 0 ]);src/modules/ForceLink/index.ts (1)
34-63: Consider adding error handling for edge cases.While the code handles null checks and division by zero, it could benefit from explicit error handling for invalid data structures.
const grouped = direction === LinkDirection.INCOMING ? data.sourceIndexToTargetIndices : data.targetIndexToSourceIndices +if (!grouped) { + console.warn('No link data available for direction:', direction) + return +} this.maxPointDegree = 0 let linkIndex = 0 -grouped?.forEach((connectedPointIndices, pointIndex) => { +grouped.forEach((connectedPointIndices, pointIndex) => {src/modules/Lines/index.ts (2)
109-112: Validate color, width, and arrow buffers before drawing
It’s good that the draw method ensures colorBuffer, widthBuffer, and arrowBuffer are initialized if they’re missing. However, if the internal data changes at runtime, ensure that subsequent calls to update these buffers occur to avoid rendering stale data.
139-141: Safe initialization of colorBuffer
The fallback to a new Float32Array for linkColors ensures no runtime error. If you anticipate custom color arrays of different sizes, consider logging or validation that ensures the array length matches the link count.src/modules/Points/index.ts (13)
23-54: Class signature and new private fields
Transitioning from node-based to point-based logic is evident here. Declaring dedicated Framebuffer and Buffer fields for color, size, sampled points, and so forth, clearly separates concerns. Verify that large arrays (e.g., colorBuffer) are disposed or updated as data changes to avoid memory leaks.
162-176: dragPointCommand ensures direct point manipulation
Drag logic, referencing the hovered point index and mouse position, is well laid out. Consider boundary checks if points can be dragged outside the viewport.
231-255: Efficient area selection approach
findPointsOnAreaSelectionCommand uses the position and size framebuffers to compute selections on GPU. This is a great performance improvement over CPU-based iteration, especially with large data sets.
257-266: clearHoveredFboCommand
Clearing is effectively handled with the shared clearFrag. If hoveredFbo is used frequently, confirm that re-creation is minimal to avoid overhead.
315-341: fillSampledPointsFboCommand
Populates the sampledPointsFbo for partial retrieval in getSampledPointPositionsMap. This technique is beneficial for flexible “zoomed out” or “sampled” analyses. Ensure that usage scenarios are well documented so devs know when to call fillSampledPointsFboCommand.
342-384: drawHighlightedCommand for highlighting
• Separate draw command for highlighted geometry is a standard approach.
• Because this is a second pass, watch performance if highlight logic triggers frequently.
403-406: updateColor
Straightforward. Buffer reuse is good. As an improvement, consider verifying array lengths to match data.pointsNumber.
410-435: updateGreyoutStatus
• Fills a texture with either 0 or 1 for each point.
• This logic is easily extensible to support more categories (e.g., multi-level highlighting).
439-463: updateSize
• Creates and updates sizeBuffer / sizeTexture.
• Good usage of a single separate texture for size data, consistent with the color approach.
• Double-check edge cases: if data.pointSizes length < data.pointsNumber, the leftover points will be zero.
482-482: trackPoints
A minimal wrapper calling trackPointsCommand. If additional logic or checks are needed, consider placing them here to maintain separation of concerns.
518-518: findPointsOnAreaSelection
Invokes the relevant command. Straightforward pass-through that can be extended with logging or event triggers if needed.
569-581: getTrackedPositionsMap
• Good usage of readPixels on trackedPositionsFbo to build a Map.
• The approach is straightforward, returning 2D coords for each tracked index.
• Consider checking for out-of-bounds or NaN in x or y if the FBO data is incomplete.
609-638: Commented-out node rescaling
Unsure if you plan to remove or refactor this code. If it’s no longer relevant, consider removing it entirely to reduce clutter. Otherwise, a separate utility for node→point transformations might be beneficial.Would you like me to open a GitHub issue to track the removal or re-implementation of the rescaling logic?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.md(2 hunks)src/modules/Clusters/force-cluster.frag(1 hunks)src/modules/Clusters/index.ts(1 hunks)src/modules/ForceLink/index.ts(2 hunks)src/modules/ForceManyBody/force-level.frag(5 hunks)src/modules/Lines/index.ts(1 hunks)src/modules/Points/index.ts(6 hunks)src/stories/1. welcome.mdx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/stories/1. welcome.mdx
- src/modules/Clusters/force-cluster.frag
- src/modules/ForceManyBody/force-level.frag
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~67-~67: Loose punctuation mark.
Context: ...aph.render() ``` - pointPositions: A Float32Array of `[x1, y1, x2, y2, ......
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~68-~68: Loose punctuation mark.
Context: ...y1, x2, y2, ..., xN, yN]. - **links**: A Float32Array of [sourceIndex1, targe...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 Markdownlint (0.37.0)
README.md
118-118: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (26)
src/modules/Clusters/index.ts (2)
1-25: LGTM! Well-structured class declaration with proper type safety
The class structure follows good practices with clear separation of public and private members, proper typing for WebGL resources, and organized imports.
32-35: Enhance cluster index validation
The current validation is incomplete compared to the previously suggested implementation.
README.md (3)
7-16: LGTM! Introduction and demo sections are well-structured.
The changes improve accessibility with proper video embedding and alt text, while maintaining clear and concise documentation.
104-104: Enhance the iOS limitation documentation.
The iOS WebGL limitation description should provide more context and potential workarounds.
112-112:
Update migration guide link in documentation section.
The migration guide link still points to the 'next' branch, which should be updated to use the main branch or a specific tag.
-🚀 [Migration Guide](https://github.com/cosmograph-org/cosmos/tree/next/cosmos-2-0-migration-notes.md)
+🚀 [Migration Guide](https://github.com/cosmograph-org/cosmos/tree/main/cosmos-2-0-migration-notes.md)Likely invalid or redundant comment.
src/modules/ForceLink/index.ts (1)
12-24: LGTM! Improved encapsulation and type safety.
The removal of generic types and addition of private modifiers improves the code's maintainability and type safety.
src/modules/Lines/index.ts (4)
5-5: No concerns with this import addition
The import of defaultConfigValues is consistent with usage throughout the file.
8-10: Class signature and new buffer field look good
Removing generic type parameters simplifies the class. The optional pointsBuffer is properly typed, though consider providing a default or a docstring clarifying usage.
18-105: Ensure initPrograms idempotency
The initPrograms method conditionally creates the drawCurveCommand if it doesn't exist. This pattern is good for re-initialization without duplicating resources. However, confirm that any potential buffer/command reinitialization (if data or config changes) is handled elsewhere (e.g., calling updatePointsBuffer).
Would you like a verification script to confirm that the drawCurveCommand is reinitialized correctly when data or settings change?
116-136: Well-structured buffer creation in updatePointsBuffer
The usage of a typed array (Float32Array) for instancePoints is optimal. If future features require partial updates, consider more granular buffer updates or subdata calls to improve performance in large-scale use cases.
src/modules/Points/index.ts (16)
2-3: Confirmed removal of d3 imports
Since the code now manages scales and extents internally, removing d3-scale and d3-array references is acceptable.
13-14: New shader imports
The fillGridWithSampledPoints shader pair is properly named and presumably used for sampling. No issues noted.
16-16: Shared buffer utility import
Using createIndexesForBuffer, createQuadBuffer, and destroyFramebuffer from Shared/buffer is consistent with the approach to unify buffer management.
18-18: Drag shader import
The addition of dragPointFrag indicates improved functionality for interactive point movement. Ensure it’s invoked only when needed to prevent unintended side effects.
Would you like assistance verifying that dragPointFrag usage doesn’t conflict with broader simulation updates?
Line range hint 55-139: updatePositions offers comprehensive initialization
- Good usage of Float32Array for building initial FBO data.
- Proper calls to destroyFramebuffer to avoid resource conflicts.
- The fallback filling with zero for velocity FBO is consistent with a stable start for simulation.
Ensure that updates to positions are triggered after config changes or new point data.
139-139: Chaining updates
Calling updateGreyoutStatus and updateSampledPointsGrid at the end of updatePositions helps keep data in sync. This is a good pattern to ensure all references are up-to-date after initialization.
145-160: updatePositionCommand is correctly guarded
The check for disableSimulation ensures the GPU-based simulation logic is optional. The use of friction, velocity, and spaceSize as uniforms is consistent with typical physics-simulation patterns.
178-225: drawCommand for point rendering
• Using the sizeBuffer and colorBuffer as attributes is straightforward.
• Summation of new attribute buffers (pointIndices, size, color) aligns with the new point-based approach.
• The blend configuration is consistent with other rendering modules.
268-302: findHoveredPoint on GPU
In a large-scale scenario, this approach scales better than CPU-based searching. Confirm that repeated calls do not degrade performance if mouse movement is constant.
304-314: clearSampledPointsFboCommand
Similar to clearHoveredFboCommand, this is an effective cleanup step. Consistency across different buffer-clearing commands is good for maintainability.
385-399: trackPointsCommand
Loads positions from the main FBO into a “trackedPositionsFbo,” presumably for advanced usage. The design is consistent with the trackPointsByIndices approach.
466-473: updateSampledPointsGrid
Potential division-by-zero is handled, defaulting to pointSamplingDistance. The derived grid shape matches the scenario for sampling. If partial sampling is used at multiple scales, ensure recalculation after resizing the screen or changing the sampling distance.
487-504: draw
• Ensures colorBuffer and sizeBuffer initialization.
• The highlight ring is drawn only if hovered or focused points exist.
• Adequate fallback usage of pointSize if a specific index’s size is missing.
514-517: drag method
Hooks into dragPointCommand. This is flexible for real-time dragging. As with all FBO swaps, watch out for concurrency if the app does multi-threaded or asynchronous data changes.
528-565: trackPointsByIndices
• Properly handles an arbitrary list of indices, writing them into trackedIndicesTexture and trackedIndicesFbo.
• The negative fill for uninitialized cells is a good sentinel.
• This approach is effective for partial or random sets of points.
584-603: getSampledPointPositionsMap
• Calls clearSampledPointsFboCommand, then fillSampledPointsFboCommand, then reads back pixel data.
• Setting isNotEmpty from the second channel is a neat trick.
• This is a robust approach for partial data retrieval.
| public initPrograms (): void { | ||
| const { reglInstance, store, data, points } = this | ||
| if (data.pointClusters === undefined) return | ||
|
|
||
| if (!this.clearCentermassCommand) { | ||
| this.clearCentermassCommand = reglInstance({ | ||
| frag: clearFrag, | ||
| vert: updateVert, | ||
| framebuffer: () => this.centermassFbo as regl.Framebuffer2D, | ||
| primitive: 'triangle strip', | ||
| count: 4, | ||
| attributes: { vertexCoord: createQuadBuffer(reglInstance) }, | ||
| }) | ||
| } | ||
| if (!this.calculateCentermassCommand) { | ||
| this.calculateCentermassCommand = reglInstance({ | ||
| frag: calculateCentermassFrag, | ||
| vert: calculateCentermassVert, | ||
| framebuffer: () => this.centermassFbo as regl.Framebuffer2D, | ||
| primitive: 'points', | ||
| count: () => data.pointsNumber ?? 0, | ||
| attributes: { | ||
| pointIndices: { | ||
| buffer: this.pointIndices, | ||
| size: 2, | ||
| }, | ||
| }, | ||
| uniforms: { | ||
| positionsTexture: () => points?.previousPositionFbo, | ||
| pointsTextureSize: () => store.pointsTextureSize, | ||
| clusterTexture: () => this.clusterFbo, | ||
| clustersTextureSize: () => this.clustersTextureSize, | ||
| }, | ||
| blend: { | ||
| enable: true, | ||
| func: { | ||
| src: 'one', | ||
| dst: 'one', | ||
| }, | ||
| equation: { | ||
| rgb: 'add', | ||
| alpha: 'add', | ||
| }, | ||
| }, | ||
| depth: { enable: false, mask: false }, | ||
| stencil: { enable: false }, | ||
| }) | ||
| } | ||
| if (!this.applyForcesCommand) { | ||
| this.applyForcesCommand = reglInstance({ | ||
| frag: forceFrag, | ||
| vert: updateVert, | ||
| framebuffer: () => points?.velocityFbo as regl.Framebuffer2D, | ||
| primitive: 'triangle strip', | ||
| count: 4, | ||
| attributes: { vertexCoord: createQuadBuffer(reglInstance) }, | ||
| uniforms: { | ||
| positionsTexture: () => points?.previousPositionFbo, | ||
| clusterTexture: () => this.clusterFbo, | ||
| centermassTexture: () => this.centermassFbo, | ||
| clusterPositionsTexture: () => this.clusterPositionsFbo, | ||
| clusterForceCoefficient: () => this.clusterForceCoefficientFbo, | ||
| alpha: () => store.alpha, | ||
| clustersTextureSize: () => this.clustersTextureSize, | ||
| clusterCoefficient: () => this.config.simulationCluster, | ||
| }, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for shader compilation failures
The shader program initialization should include error handling to catch and report shader compilation failures.
Consider wrapping the regl command creation in try-catch blocks:
if (!this.calculateCentermassCommand) {
+ try {
this.calculateCentermassCommand = reglInstance({
frag: calculateCentermassFrag,
vert: calculateCentermassVert,
// ... existing configuration
})
+ } catch (error) {
+ console.error('Failed to compile centermass calculation shaders:', error)
+ throw error
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| clusterPositions[cluster * 4 + 0] = data.clusterPositions[cluster * 2 + 0] ?? -1 | ||
| clusterPositions[cluster * 4 + 1] = data.clusterPositions[cluster * 2 + 1] ?? -1 | ||
| } |
There was a problem hiding this comment.
Add bounds checking for cluster positions array access
The array access at data.clusterPositions[cluster * 2 + 0] and data.clusterPositions[cluster * 2 + 1] could potentially access undefined indices if data.clusterPositions length is less than clusterNumber * 2.
Apply this diff to add bounds checking:
- clusterPositions[cluster * 4 + 0] = data.clusterPositions[cluster * 2 + 0] ?? -1
- clusterPositions[cluster * 4 + 1] = data.clusterPositions[cluster * 2 + 1] ?? -1
+ const posIndex = cluster * 2
+ if (posIndex + 1 < (data.clusterPositions?.length ?? 0)) {
+ clusterPositions[cluster * 4 + 0] = data.clusterPositions[posIndex] ?? -1
+ clusterPositions[cluster * 4 + 1] = data.clusterPositions[posIndex + 1] ?? -1
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| clusterPositions[cluster * 4 + 0] = data.clusterPositions[cluster * 2 + 0] ?? -1 | |
| clusterPositions[cluster * 4 + 1] = data.clusterPositions[cluster * 2 + 1] ?? -1 | |
| } | |
| const posIndex = cluster * 2 | |
| if (posIndex + 1 < (data.clusterPositions?.length ?? 0)) { | |
| clusterPositions[cluster * 4 + 0] = data.clusterPositions[posIndex] ?? -1 | |
| clusterPositions[cluster * 4 + 1] = data.clusterPositions[posIndex + 1] ?? -1 | |
| } |
|
|
||
| > **Warning** | ||
| > If you're going to create a new graph within the same HTML canvas element that already has a graph, **destroy** the previous graph using `graph.destroy()` to prevent unexpected glitches. | ||
| Check the [Migration Guide](https://github.com/cosmograph-org/cosmos/cosmos-2-0-migration-notes.md) for details. |
There was a problem hiding this comment.
💡 Codebase verification
Fix incorrect migration guide links in README.md
The migration guide exists in the repository root, but the README.md contains two different and incorrect links to it:
- Line 84:
https://github.com/cosmograph-org/cosmos/cosmos-2-0-migration-notes.md - Another line:
https://github.com/cosmograph-org/cosmos/tree/next/cosmos-2-0-migration-notes.md
Both should be updated to simply point to the local file: ./cosmos-2-0-migration-notes.md
🔗 Analysis chain
Verify the migration guide link.
The current link might be incorrect. It should point to the file in the repository's main branch.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the migration guide file exists and is accessible
# Check if file exists in the repository
if [ -f "cosmos-2-0-migration-notes.md" ]; then
echo "Migration guide exists at repository root"
else
echo "Migration guide not found at repository root"
# Check if it exists in other common locations
fd -t f "cosmos-2-0-migration-notes.md"
fi
Length of output: 135
Script:
#!/bin/bash
# Let's verify if the link path in README.md is correct
# and check the actual content of both files to ensure they match
# First, let's see the actual content around the link in README.md
echo "=== README.md content ==="
rg -A 1 -B 1 "Migration Guide" README.md
# Then, let's check the content of the migration guide
echo -e "\n=== Migration guide content ==="
head -n 5 cosmos-2-0-migration-notes.md
Length of output: 861
| if (!this.linkFirstIndicesAndAmountTexture) this.linkFirstIndicesAndAmountTexture = reglInstance.texture() | ||
| this.linkFirstIndicesAndAmountTexture({ | ||
| data: this.linkFirstIndicesAndAmount, | ||
| shape: [pointsTextureSize, pointsTextureSize, 4], | ||
| type: 'float', | ||
| }) | ||
| this.indicesFbo = reglInstance.framebuffer({ | ||
| color: reglInstance.texture({ | ||
| data: this.indices, | ||
| shape: [linksTextureSize, linksTextureSize, 4], | ||
| type: 'float', | ||
| }), | ||
| depth: false, | ||
| stencil: false, | ||
| if (!this.linkFirstIndicesAndAmountFbo) { | ||
| this.linkFirstIndicesAndAmountFbo = reglInstance.framebuffer({ | ||
| color: this.linkFirstIndicesAndAmountTexture, | ||
| depth: false, | ||
| stencil: false, | ||
| }) | ||
| } | ||
|
|
||
| if (!this.indicesTexture) this.indicesTexture = reglInstance.texture() | ||
| this.indicesTexture({ | ||
| data: this.indices, | ||
| shape: [linksTextureSize, linksTextureSize, 4], | ||
| type: 'float', | ||
| }) | ||
| this.biasAndStrengthFbo = reglInstance.framebuffer({ | ||
| color: reglInstance.texture({ | ||
| data: linkBiasAndStrengthState, | ||
| shape: [linksTextureSize, linksTextureSize, 4], | ||
| type: 'float', | ||
| }), | ||
| depth: false, | ||
| stencil: false, | ||
| if (!this.indicesFbo) { | ||
| this.indicesFbo = reglInstance.framebuffer({ | ||
| color: this.indicesTexture, | ||
| depth: false, | ||
| stencil: false, | ||
| }) | ||
| } | ||
|
|
||
| if (!this.biasAndStrengthTexture) this.biasAndStrengthTexture = reglInstance.texture() | ||
| this.biasAndStrengthTexture({ | ||
| data: linkBiasAndStrengthState, | ||
| shape: [linksTextureSize, linksTextureSize, 4], | ||
| type: 'float', | ||
| }) | ||
| this.randomDistanceFbo = reglInstance.framebuffer({ | ||
| color: reglInstance.texture({ | ||
| data: linkDistanceState, | ||
| shape: [linksTextureSize, linksTextureSize, 4], | ||
| type: 'float', | ||
| }), | ||
| depth: false, | ||
| stencil: false, | ||
| if (!this.biasAndStrengthFbo) { | ||
| this.biasAndStrengthFbo = reglInstance.framebuffer({ | ||
| color: this.biasAndStrengthTexture, | ||
| depth: false, | ||
| stencil: false, | ||
| }) | ||
| } | ||
|
|
||
| if (!this.randomDistanceTexture) this.randomDistanceTexture = reglInstance.texture() | ||
| this.randomDistanceTexture({ | ||
| data: linkDistanceState, | ||
| shape: [linksTextureSize, linksTextureSize, 4], | ||
| type: 'float', | ||
| }) | ||
| if (!this.randomDistanceFbo) { | ||
| this.randomDistanceFbo = reglInstance.framebuffer({ | ||
| color: this.randomDistanceTexture, | ||
| depth: false, | ||
| stencil: false, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for texture initialization.
Consider adding error handling and cleanup logic for texture initialization failures.
+private initTexture(
+ texture: regl.Texture2D | undefined,
+ data: Float32Array,
+ shape: [number, number, number]
+): regl.Texture2D {
+ try {
+ if (!texture) texture = this.reglInstance.texture()
+ texture({
+ data,
+ shape,
+ type: 'float',
+ })
+ return texture
+ } catch (error) {
+ console.error('Failed to initialize texture:', error)
+ throw error
+ }
+}
if (!this.linkFirstIndicesAndAmountTexture) this.linkFirstIndicesAndAmountTexture = reglInstance.texture()
-this.linkFirstIndicesAndAmountTexture({
- data: this.linkFirstIndicesAndAmount,
- shape: [pointsTextureSize, pointsTextureSize, 4],
- type: 'float',
-})
+this.linkFirstIndicesAndAmountTexture = this.initTexture(
+ this.linkFirstIndicesAndAmountTexture,
+ this.linkFirstIndicesAndAmount,
+ [pointsTextureSize, pointsTextureSize, 4]
+)Committable suggestion skipped: line range outside the PR's diff.
First steps for the refactor 🐣
Set data low level refactor 🐥
visualized by Csmograph.app
* Storybook: Showing source code examples * Storybook: Code re-organization --------- Co-authored-by: Stukova Olya <[email protected]>
…vector calculations With the assistance of CodeRabbit 🐇
With the assistance of CodeRabbit 🐇
There was a problem hiding this comment.
Actionable comments posted: 13
🔭 Outside diff range comments (1)
src/config.ts (1)
Line range hint
535-540: Improve type safety in configuration initializationThe
initmethod could benefit from stronger type checking.- public init (config: GraphConfigInterface): void { + public init (config: Partial<GraphConfigInterface>): void { + if (!config || typeof config !== 'object') { + console.error('Invalid configuration object') + return + } (Object.keys(config) as (keyof GraphConfigInterface)[]) .forEach(configParameter => { + if (configParameter in this) { this.deepMergeConfig(this.getConfig(), config, configParameter) + } else { + console.warn(`Unknown configuration parameter: ${String(configParameter)}`) + } }) }
♻️ Duplicate comments (6)
src/stories/create-cluster-labels.ts (1)
39-47:⚠️ Potential issueImprove null handling in coordinate extraction.
The current implementation silently defaults null coordinates to 0, which could lead to misleading visualization. Consider hiding labels with invalid coordinates.
- const clusterPosition = clusterPositions.slice(i * 2, i * 2 + 2) - const x = clusterPosition[0] - const y = clusterPosition[1] - const clusterLabelDiv = clusterLabelDivs[i] as HTMLDivElement - const screenXY = graph.spaceToScreenPosition([x ?? 0, y ?? 0]) + const [x, y] = clusterPositions.slice(i * 2, i * 2 + 2) + if (x === null || y === null) { + // Hide label when coordinates are invalid + clusterLabelDivs[i].style.display = 'none' + continue + } + clusterLabelDivs[i].style.display = '' + const screenXY = graph.spaceToScreenPosition([x, y])src/stories/generate-mesh-data.ts (1)
39-40:⚠️ Potential issueFix incorrect usage of
scaleLinearwhen setting domain and range.The
scaleLinearfunction should be initialized without arguments and configured using the.domain()and.range()methods.Apply this diff to correct the usage:
-const radius = scaleLinear(radialness) -radius.domain([0, nClusters]) +const radius = scaleLinear() + .domain([0, nClusters]) + .range(radialness)src/modules/ForceCenter/index.ts (1)
19-35:⚠️ Potential issueAdd destroy() method to prevent memory leaks
WebGL resources (textures, framebuffers, buffers) should be properly disposed of when no longer needed to prevent memory leaks.
Add a destroy method to clean up resources:
+ public destroy(): void { + this.centermassTexture?.destroy() + this.centermassFbo?.destroy() + this.pointIndices?.destroy() + this.centermassTexture = undefined + this.centermassFbo = undefined + this.pointIndices = undefined + }README.md (1)
84-84:⚠️ Potential issueFix inconsistent migration guide links
The README contains two different and incorrect links to the migration guide:
- Line 84:
https://github.com/cosmograph-org/cosmos/cosmos-2-0-migration-notes.md- Line 112:
https://github.com/cosmograph-org/cosmos/tree/next/cosmos-2-0-migration-notes.mdBoth should be updated to use the local file path:
-Check the [Migration Guide](https://github.com/cosmograph-org/cosmos/cosmos-2-0-migration-notes.md) for details. +Check the [Migration Guide](./cosmos-2-0-migration-notes.md) for details. -🚀 [Migration Guide](https://github.com/cosmograph-org/cosmos/tree/next/cosmos-2-0-migration-notes.md) +🚀 [Migration Guide](./cosmos-2-0-migration-notes.md)Also applies to: 112-112
src/modules/ForceManyBodyQuadtree/index.ts (1)
12-19:⚠️ Potential issueResource cleanup mechanism needed
While the texture and FBO management improvements are good, the removal of the
destroymethod raises concerns about resource cleanup.Consider reintroducing a cleanup mechanism to prevent memory leaks. Here's a suggested implementation:
+ public destroy (): void { + this.levelsTextures.forEach(texture => texture.destroy()) + this.levelsFbos.forEach(fbo => fbo.destroy()) + this.randomValuesTexture?.destroy() + this.randomValuesFbo?.destroy() + this.pointIndices?.destroy() + }Also applies to: 27-46
src/index.ts (1)
802-808:⚠️ Potential issueReorder cleanup operations in destroy method
The current implementation clears the canvas after destroying the regl instance, which may not have the intended effect.
Reorder the operations to ensure proper cleanup:
+ this.reglInstance.clear({ + color: this.store.backgroundColor, + depth: 1, + stencil: 0, + }) this.reglInstance.destroy() - this.reglInstance.clear({ - color: this.store.backgroundColor, - depth: 1, - stencil: 0, - })
🧹 Nitpick comments (39)
src/stories/create-cluster-labels.ts (1)
5-8: Add validation for cluster positions array length.The calculation of
nClustersassumes the array length is even (as positions are [x,y] pairs). Consider adding validation to prevent issues with malformed data.return function updateClusterLabels (graph: Graph): void { const clusterPositions = graph.getClusterPositions() + if (clusterPositions.length % 2 !== 0) { + console.warn('Cluster positions array length must be even') + return + } const nClusters = clusterPositions.length / 2 if (nClusters === 0) returnCODE_OF_CONDUCT.md (2)
19-36: Enhance clarity in reporting proceduresThe reporting procedures are well-defined, but could benefit from some minor improvements in readability:
- Add a comma after "Cosmos" in line 19
- Consider replacing the wordy phrase "with regard to" with "regarding" or "concerning" in both reporting sections
Apply these changes for improved readability:
-For reporting issues in spaces related to Cosmos please use the email +For reporting issues in spaces related to Cosmos, please use the email `[email protected]`. Cosmos handles CoC issues related to the spaces that it maintains. Projects maintainers commit to: -- maintain the confidentiality with regard to the reporter of an incident +- maintain confidentiality regarding the reporter of an incident ### Foundation Spaces ... -- maintain the confidentiality with regard to the reporter of an incident +- maintain confidentiality regarding the reporter of an incident🧰 Tools
🪛 LanguageTool
[typographical] ~19-~19: Consider adding a comma here.
Context: ...rting issues in spaces related to Cosmos please use the email[email protected]. Co...(PLEASE_COMMA)
[style] ~23-~23: ‘with regard to’ might be wordy. Consider a shorter alternative.
Context: ...mit to: - maintain the confidentiality with regard to the reporter of an incident - to partic...(EN_WORDINESS_PREMIUM_WITH_REGARD_TO)
[uncategorized] ~30-~30: Do not mix variants of the same word (‘organization’ and ‘organisation’) within a single text.
Context: ...example, repositories within the OpenJS organization, use the email `[email protected]...(EN_WORD_COHERENCY)
[style] ~34-~34: ‘with regard to’ might be wordy. Consider a shorter alternative.
Context: ...its to: - maintain the confidentiality with regard to the reporter of an incident - to partic...(EN_WORDINESS_PREMIUM_WITH_REGARD_TO)
70-87: Maintain consistent markdown list styleThe document uses both asterisks and dashes for unordered lists. For consistency, use dashes throughout the document as they're used in other sections.
Apply this change to the lists in the "Our Standards" section:
-* Demonstrating empathy and kindness toward other people -* Being respectful of differing opinions, viewpoints, and experiences -* Giving and gracefully accepting constructive feedback -* Accepting responsibility and apologizing to those affected by our mistakes, +- Demonstrating empathy and kindness toward other people +- Being respectful of differing opinions, viewpoints, and experiences +- Giving and gracefully accepting constructive feedback +- Accepting responsibility and apologizing to those affected by our mistakes, ... -* The use of sexualized language or imagery, and sexual attention or advances of -* Trolling, insulting or derogatory comments, and personal or political attacks -* Public or private harassment -* Publishing others' private information, such as a physical or email address, -* Other conduct which could reasonably be considered inappropriate in a +- The use of sexualized language or imagery, and sexual attention or advances of +- Trolling, insulting or derogatory comments, and personal or political attacks +- Public or private harassment +- Publishing others' private information, such as a physical or email address, +- Other conduct which could reasonably be considered inappropriate in a🧰 Tools
🪛 LanguageTool
[style] ~82-~82: Try using a synonym here to strengthen your wording.
Context: ...ind * Trolling, insulting or derogatory comments, and personal or political attacks * Pu...(COMMENT_REMARK)
🪛 Markdownlint (0.37.0)
70-70: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
71-71: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
72-72: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
73-73: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
75-75: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
80-80: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
82-82: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
83-83: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
84-84: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
86-86: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
src/stories/create-cosmos.ts (3)
18-24: Clean up commented canvas-related code.The commented canvas code suggests a potential architectural change from canvas to div-based rendering. If canvas is no longer used, these comments should be removed for cleaner code.
export const createCosmos = (props: CosmosStoryProps): { div: HTMLDivElement; graph: Graph} => { const div = document.createElement('div') - // const canvas = document.createElement('canvas') div.style.height = '100vh' div.style.width = '100%' - // div.appendChild(canvas)
25-47: Consider adding documentation for configuration parameters.The configuration includes several simulation physics parameters (gravity, friction, spring, etc.) that would benefit from documentation explaining their effects and recommended ranges.
Consider adding JSDoc comments for the configuration object:
const config: GraphConfigInterface = { + /** Background color of the graph visualization */ backgroundColor: '#212C42', + /** Default point size in pixels */ pointSize: 3, // ... add documentation for other properties + /** Simulation gravity force (0.01-0.1 recommended) */ simulationGravity: 0.02, + /** Simulation friction coefficient (0.1-0.9 recommended) */ simulationFriction: 0.7, // ... document other simulation parameters
64-68: Consider making initial zoom configurable.The zoom value of 0.9 is hardcoded. Consider making this configurable through the props interface for more flexibility.
+ type CosmosStoryProps = GraphConfigInterface & { + initialZoom?: number; // ... existing properties } // ... in createCosmos function - graph.zoom(0.9) + graph.zoom(props.initialZoom ?? 0.9)src/stories/generate-mesh-data.ts (6)
5-7: Consider improving the precision and reliability of the random number generation.The current implementation could be enhanced to:
- Handle edge cases where min equals max
- Ensure consistent precision in the output
Consider this implementation:
-function getRandom (min: number, max: number): number { - return Math.random() * (max - min) + min +function getRandom (min: number, max: number): number { + if (min === max) return min; + return Number((Math.random() * (max - min) + min).toFixed(6)) }
23-23: Remove commented out code.There are several instances of commented out
linkStrengthrelated code. If this feature is planned for future implementation, consider tracking it in an issue instead.Also applies to: 94-94, 104-104, 115-115
50-56: Extract magic numbers into named constants.The cluster generation logic uses magic numbers that should be extracted into named constants for better maintainability and configuration.
Consider this refactor:
+const SPACE_SIZE = 8192 +const ANGLE_MULTIPLIER = 15 +const HALF_SPACE = SPACE_SIZE / 2 -const spaceSize = 8192 - for (let clusterIndex = 0; clusterIndex < nClusters; clusterIndex += 1) { - const [x, y] = getPositionOnCircle(radius(clusterIndex), 15 * Math.PI * (clusterIndex / nClusters), spaceSize / 2) + const [x, y] = getPositionOnCircle( + radius(clusterIndex), + ANGLE_MULTIPLIER * Math.PI * (clusterIndex / nClusters), + HALF_SPACE + ) clusterPositions[clusterIndex * 2] = x clusterPositions[clusterIndex * 2 + 1] = y }
59-60: Extract position range constants and consider parameterizing the spread.The point position calculation uses magic numbers (0.495, 0.505) for the spread range. This should be configurable.
Consider this improvement:
+const POSITION_SPREAD = 0.005 // (0.505 - 0.495) / 2 +const CENTER_POSITION = 0.5 -const x = spaceSize * getRandom(0.495, 0.505) -const y = spaceSize * getRandom(0.495, 0.505) +const x = spaceSize * getRandom(CENTER_POSITION - POSITION_SPREAD, CENTER_POSITION + POSITION_SPREAD) +const y = spaceSize * getRandom(CENTER_POSITION - POSITION_SPREAD, CENTER_POSITION + POSITION_SPREAD)
81-89: Consider extracting link generation logic into a separate function.The link generation logic is complex and could be more readable if extracted into a dedicated function.
Consider this refactor:
+function shouldCreateLink( + pointIndex: number, + targetIndex: number, + n: number, + m: number, + wholeness: number +): boolean { + const pointLine = Math.floor(pointIndex / n) + const targetLine = Math.floor(targetIndex / n) + + if (targetIndex === pointIndex + 1) { + return pointLine === targetLine && Math.random() < wholeness + } + + if (targetIndex === pointIndex + n) { + return targetLine < m && Math.random() < wholeness + } + + return false +} -if (pointLine === nextPointLine && Math.random() < wholeness) { - links.push(pointIndex) - links.push(nextPointIndex) -} - -if (bottomPointLine < m && Math.random() < wholeness) { - links.push(pointIndex) - links.push(bottomPointIndex) -} +if (shouldCreateLink(pointIndex, nextPointIndex, n, m, wholeness)) { + links.push(pointIndex, nextPointIndex) +} + +if (shouldCreateLink(pointIndex, bottomPointIndex, n, m, wholeness)) { + links.push(pointIndex, bottomPointIndex) +}
96-104: Improve type safety and extract configuration constants.The link properties generation has a few areas for improvement:
- Unnecessary type assertion for
sourcePointIndex- Hardcoded alpha value
- Hardcoded link width range
Consider these improvements:
+const LINK_OPACITY = 0.9 +const MIN_LINK_WIDTH = 0.1 +const MAX_LINK_WIDTH = 0.5 -const sourcePointIndex = links[i * 2] as number +const sourcePointIndex = links[i * 2] const rgba = getRgbaColor(pointColorScale(sourcePointIndex % nClusters)) linkColors[i * 4 + 0] = rgba[0] linkColors[i * 4 + 1] = rgba[1] linkColors[i * 4 + 2] = rgba[2] -linkColors[i * 4 + 3] = 0.9 +linkColors[i * 4 + 3] = LINK_OPACITY -linkWidths[i] = getRandom(0.1, 0.5) +linkWidths[i] = getRandom(MIN_LINK_WIDTH, MAX_LINK_WIDTH).github/SECURITY.md (1)
5-5: Enhance version support clarityThe current statement about supported versions is too brief. Consider expanding it to include:
- Current version number
- End-of-life policy
- Migration guidance for older versions
-Security updates are applied only to the latest release. +## Supported Versions + +| Version | Supported | End of Support | +| ------- | ------------------ | -------------- | +| 2.0.x | :white_check_mark: | Current | +| < 2.0 | :x: | 2024-12-31 | + +Security updates are applied only to the latest 2.0.x release. Users on older versions should upgrade to the latest release.src/modules/core-module.ts (1)
Line range hint
14-26: Document migration steps for breaking changesThe constructor signature changes require updates in all consuming code.
Consider adding migration documentation in
MIGRATION.mdto guide users through:
- Updating type annotations
- Removing generic type parameters
- Adapting custom node/link type implementations
src/modules/Shared/buffer.ts (1)
Line range hint
11-20: Consider performance optimization for large texturesThe function creates a new Float32Array for each call, which could be memory-intensive for large texture sizes.
Consider these optimizations:
- Add input validation for texture size
- Consider using a pooling mechanism for large arrays
- Add JSDoc to document memory implications
+/** + * Creates an array of texture coordinates for a given texture size. + * @param textureSize - Size of the texture (width/height) + * @returns Float32Array of size textureSize^2 * 2 + * @throws {Error} If textureSize is <= 0 or exceeds maximum supported size + */ export function createIndexesForBuffer (textureSize: number): Float32Array { + if (textureSize <= 0 || textureSize > 16384) { + throw new Error('Invalid texture size') + } const indexes = new Float32Array(textureSize * textureSize * 2)src/modules/ForceGravity/index.ts (1)
12-27: Consider extracting common initialization patternThe initialization logic is nearly identical to ForceMouse. Consider extracting this pattern into a shared base class or utility.
+ // In CoreModule + protected initReglCommand(config: { + frag: string, + uniforms: Record<string, any>, + framebuffer: () => regl.Framebuffer2D + }): void { + if (!this.runCommand) { + this.runCommand = this.reglInstance({ + vert: updateVert, + primitive: 'triangle strip', + count: 4, + attributes: { vertexCoord: createQuadBuffer(this.reglInstance) }, + ...config + }) + } + }src/variables.ts (1)
11-11: Consider impact of increased spaceSizeThe spaceSize has been doubled from 4096 to 8192. This could affect memory usage and performance for large graphs.
Consider documenting the performance implications of this change and providing guidance on when to adjust this value.
src/helper.ts (2)
33-35: Consider adding JSDoc for rgbToBrightnessThe
rgbToBrightnessfunction implements the standard luminance formula, but lacks documentation explaining the formula and expected input ranges.Add JSDoc comment:
+/** + * Calculates perceived brightness using the standard luminance formula + * @param r Red component (0-1) + * @param g Green component (0-1) + * @param b Blue component (0-1) + * @returns Perceived brightness value (0-1) + */ export function rgbToBrightness(r: number, g: number, b: number): number { return 0.2126 * r + 0.7152 * g + 0.0722 * b }
50-52: Consider enhancing isNumber type safetyThe
isNumberfunction is a type guard but isn't declared as one.Consider this improvement:
-export function isNumber(value: number | undefined | null | typeof NaN): boolean { +export function isNumber(value: unknown): value is number { return value !== undefined && value !== null && !Number.isNaN(value) && typeof value === 'number' }src/modules/Points/find-hovered-point.vert (2)
22-30: Add documentation for the point size calculation logicThe
calculatePointSizefunction implements important scaling logic but lacks documentation explaining the rationale behind the scaling factors and the relationship betweenratio,transformationMatrix, andmaxPointSize.+// Calculates the final point size based on the base size and zoom level +// ratio: Device pixel ratio for high DPI displays +// transformationMatrix[0][0]: Current zoom scale +// Returns: Clamped point size that scales with zoom based on configuration float calculatePointSize(float size) { float pSize; if (scalePointsOnZoom) { pSize = size * ratio * transformationMatrix[0][0]; } else { pSize = size * ratio * min(5.0, max(1.0, transformationMatrix[0][0] * 0.01)); } return min(pSize, maxPointSize * ratio); }
37-52: Consider optimizing the hover detection calculationThe current implementation performs hover detection for all points. Consider adding an early exit condition when the point is far from the mouse position to avoid unnecessary calculations.
vec2 pointScreenPosition = (finalPosition.xy + 1.0) * screenSize / 2.0; rgba = vec4(0.0); gl_Position = vec4(0.5, 0.5, 0.0, 1.0); + + // Early exit for points far from mouse + vec2 mouseDistance = abs(pointScreenPosition - mousePosition); + if (mouseDistance.x > pointRadius / ratio || mouseDistance.y > pointRadius / ratio) { + return; + } // Check if the mouse is within the point radius if (euclideanDistance(pointScreenPosition.x, mousePosition.x, pointScreenPosition.y, mousePosition.y) < pointRadius / ratio) {cosmos-2-0-migration-notes.md (2)
77-91: Add canvas size management detailsThe switch from canvas to div initialization is well documented, but it's missing information about how canvas size is managed in the new version.
const div = document.getElementById('myDiv') const graph = new Graph(div, config) + +// Canvas size management +// The graph will automatically create and manage a canvas element within the div +// The canvas will resize automatically when the div is resized +// You can control the size through CSS: +div.style.width = '800px'; +div.style.height = '600px';🧰 Tools
🪛 Markdownlint (0.37.0)
77-77: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
81-81: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
87-87: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
93-98: Enhance the summary with performance implicationsThe summary should include information about the performance benefits of the new data handling approach.
Add the following points to the summary:
- Performance benefits of using typed arrays (Float32Array)
- Reduced memory usage due to direct WebGL buffer usage
- Improved rendering speed with texture-based data storage
🧰 Tools
🪛 Markdownlint (0.37.0)
93-93: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
src/modules/Store/index.ts (1)
45-50: Consider deferring DOM updates in backgroundColor setterThe backgroundColor setter performs synchronous DOM updates which could cause performance issues if called frequently. Additionally, there's a potential race condition if the div property is modified concurrently.
Consider deferring DOM updates:
public set backgroundColor (color: [number, number, number, number]) { this._backgroundColor = color + requestAnimationFrame(() => { const brightness = rgbToBrightness(color[0], color[1], color[2]) document.documentElement.style.setProperty('--cosmos-attribution-color', brightness > 0.65 ? 'black' : 'white') if (this.div) this.div.style.backgroundColor = `rgba(${color[0] * 255}, ${color[1] * 255}, ${color[2] * 255}, ${color[3]})` + }) }src/modules/ForceManyBodyQuadtree/index.ts (1)
76-120: Consider caching texture size calculationsThe
levelTextureSizecalculation in uniforms could be cached to avoid repeatedMath.powcalls during rendering.+ private getLevelTextureSize (level: number): number { + return Math.pow(2, level + 1) + } public initPrograms (): void { // ... existing code ... uniforms: { // ... other uniforms ... - levelTextureSize: (_: regl.DefaultContext, props: { levelTextureSize: number }) => props.levelTextureSize, + levelTextureSize: (_: regl.DefaultContext, props: { level: number }) => this.getLevelTextureSize(props.level), },src/stories/2. configuration.mdx (2)
8-35: Minor grammar improvements neededConsider these grammatical improvements:
- Line 10: Add a comma before "and" in "initialization and it can't"
- Line 27: Add a comma after "Otherwise"
- Line 31: Simplify "whether or not" to just "whether"
🧰 Tools
🪛 LanguageTool
[uncategorized] ~10-~10: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...applied only on component initialization and it can't be changed using the `setConfi...(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~27-~27: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...ue, links are rendered as curved lines. Otherwise as straight lines |false| | curvedL...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[style] ~31-~31: Consider shortening this phrase to just ‘whether’, unless you mean ‘regardless of whether’.
Context: ... default link arrow value that controls whether or not to display link arrows |false| | li...(WHETHER)
[uncategorized] ~35-~35: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...applied only on component initialization and it can't be changed using the `setConfi...(COMMA_COMPOUND_SENTENCE)
37-54: Add missing articleAdd "the" before "Cosmos layout algorithm" in line 39 for better readability:
-Cosmos layout algorithm was inspired by +The Cosmos layout algorithm was inspired by🧰 Tools
🪛 LanguageTool
[uncategorized] ~38-~38: You might be missing the article “the” here.
Context: ...false| ## Simulation configuration Cosmos layout algorithm was inspired by the [d...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
src/modules/Lines/index.ts (2)
17-105: Consider extracting shader configuration for better maintainabilityThe shader configuration is quite complex with multiple attributes and uniforms. Consider extracting it into a separate method for better maintainability.
+ private getShaderConfig(): regl.DrawConfig { + const { config, store, points } = this + return { + vert: drawLineVert, + frag: drawLineFrag, + attributes: { + // ... existing attributes + }, + uniforms: { + // ... existing uniforms + }, + blend: { + // ... existing blend config + }, + depth: { + // ... existing depth config + }, + count: () => this.curveLineGeometry?.length ?? 0, + instances: () => this.data.linksNumber ?? 0, + primitive: 'triangle strip', + } + } public initPrograms(): void { const { reglInstance } = this if (!this.drawCurveCommand) { - this.drawCurveCommand = reglInstance({ - // ... existing configuration - }) + this.drawCurveCommand = reglInstance(this.getShaderConfig()) } }
138-153: Reduce code duplication in buffer update methodsThe update methods follow the same pattern. Consider extracting the common logic into a utility method.
+ private updateBuffer( + buffer: regl.Buffer | undefined, + data: Float32Array | undefined, + defaultValue: Float32Array = new Float32Array() + ): void { + const { reglInstance } = this + if (!buffer) buffer = reglInstance.buffer(0) + buffer(data ?? defaultValue) + } public updateColor(): void { - const { reglInstance, data } = this - if (!this.colorBuffer) this.colorBuffer = reglInstance.buffer(0) - this.colorBuffer(data.linkColors ?? new Float32Array()) + this.updateBuffer(this.colorBuffer, data.linkColors) } public updateWidth(): void { - const { reglInstance, data } = this - if (!this.widthBuffer) this.widthBuffer = reglInstance.buffer(0) - this.widthBuffer(data.linkWidths ?? new Float32Array()) + this.updateBuffer(this.widthBuffer, data.linkWidths) } public updateArrow(): void { - const { reglInstance, data } = this - if (!this.arrowBuffer) this.arrowBuffer = reglInstance.buffer(0) - this.arrowBuffer(data.linkArrows ?? new Float32Array()) + this.updateBuffer(this.arrowBuffer, data.linkArrows) }src/modules/Clusters/index.ts (2)
10-25: Add JSDoc documentation for better code maintainabilityConsider adding documentation to explain the purpose of each property and their relationships.
+ /** + * Manages cluster-based force calculations and rendering. + * Extends CoreModule to integrate with the graph visualization system. + */ export class Clusters extends CoreModule { + /** Framebuffer for storing cluster center of mass calculations */ public centermassFbo: regl.Framebuffer2D | undefined + /** Framebuffer for storing cluster membership information */ private clusterFbo: regl.Framebuffer2D | undefined // Add documentation for remaining properties
124-192: Improve shader initialization robustnessExtract shader configurations and add error handling for better maintainability and reliability.
+ private createShaderCommand( + config: regl.DrawConfig, + name: string + ): regl.DrawCommand { + try { + return this.reglInstance(config) + } catch (error) { + console.error(`Failed to create ${name} shader:`, error) + throw error + } + } public initPrograms(): void { const { reglInstance, store, data, points } = this if (data.pointClusters === undefined) return if (!this.clearCentermassCommand) { - this.clearCentermassCommand = reglInstance({ + const config = { frag: clearFrag, vert: updateVert, // ... existing configuration - }) + } + this.clearCentermassCommand = this.createShaderCommand(config, 'clear centermass') } // Apply similar pattern to other commandssrc/modules/ForceManyBody/index.ts (1)
29-40: Consider extracting FBO configuration into a separate methodThe FBO creation and configuration logic is repeated for each level. Consider extracting this into a reusable method to improve maintainability and reduce code duplication.
+ private configureFbo(fbo: regl.Framebuffer2D, size: number): void { + fbo({ + shape: [size, size], + colorType: 'float', + depth: false, + stencil: false, + }) + } for (let i = 0; i < this.quadtreeLevels; i += 1) { const levelTextureSize = Math.pow(2, i + 1) if (!this.levelsFbos.has(`level[${i}]`)) { this.levelsFbos.set(`level[${i}]`, reglInstance.framebuffer()) } const fbo = this.levelsFbos.get(`level[${i}]`) - if (fbo) { - fbo({ - shape: [levelTextureSize, levelTextureSize], - colorType: 'float', - depth: false, - stencil: false, - }) - } + if (fbo) this.configureFbo(fbo, levelTextureSize) }src/modules/GraphData/index.ts (2)
4-12: Consider using TypeScript enums or const objects for default valuesThe input properties could benefit from type-safe default values using enums or const objects.
const enum DefaultValues { PointColor = [179, 179, 179, 255], // #b3b3b3 PointSize = 4, LinkColor = [102, 102, 102, 255], // #666666 LinkWidth = 1 }
276-280: Improve null coalescing operator usageThe degree calculation could be simplified using the nullish coalescing operator.
- this.inDegree[i] = this.targetIndexToSourceIndices?.[i]?.length ?? 0 - this.outDegree[i] = this.sourceIndexToTargetIndices?.[i]?.length ?? 0 - this.degree[i] = (this.inDegree[i] ?? 0) + (this.outDegree[i] ?? 0) + this.inDegree[i] = this.targetIndexToSourceIndices?.[i]?.length || 0 + this.outDegree[i] = this.sourceIndexToTargetIndices?.[i]?.length || 0 + this.degree[i] = this.inDegree[i] + this.outDegree[i]src/config.ts (2)
18-21: Update TODO comment with more specific detailsThe TODO comment about rethinking the
disableSimulationparameter lacks context and specific action items.Consider updating the comment to:
- * TODO: rethink the logic of `disableSimulation` param 👇. + * TODO: Consider replacing `disableSimulation` with a more granular approach: + * - Add separate flags for different simulation aspects + * - Allow dynamic enabling/disabling of simulation + * - Consider performance implications
552-554: Consider making getConfig method publicThe
getConfigmethod could be useful for external configuration validation and inspection.- private getConfig (): GraphConfigInterface { + public getConfig (): Readonly<GraphConfigInterface> { return this }src/stories/3. api-reference.mdx (2)
296-296: Fix typo in documentationRemove the duplicate word "method" in the sentence.
-This method method is used to convert X and Y point coordinates +This method is used to convert X and Y point coordinates🧰 Tools
🪛 LanguageTool
[duplication] ~296-~296: Possible typo: you repeated a word
Context: ...nPosition(coordinates) This method method is used to convert X and Y point coordi...(ENGLISH_WORD_REPEAT_RULE)
394-398: Enhance return type documentationThe
getPointPositionsmethod documentation should specify:
- Return type and format
- Error handling behavior
- Return value for edge cases (empty graph, etc.)
Add the following to the documentation:
/** * Get current X and Y coordinates of all points in the graph as an array of numbers. +* @returns {number[]} Array of coordinates in format [x1, y1, x2, y2, ...]. Returns empty array if graph has no points. +* @throws {Error} If the points buffer cannot be read. */src/index.ts (1)
960-964: Optimize point dragging performanceThe current implementation runs the drag function twice with a comment explaining it prevents sudden jumps. This is a code smell and might indicate an underlying issue in the drag calculation.
Instead of running the drag function twice, consider:
- Investigating why points jump and fix the root cause
- If the double call is necessary, implement it as a configurable parameter:
+private readonly DRAG_ITERATIONS = 2; // Make this configurable if (this.dragInstance.isActive) { - // To prevent the dragged point from suddenly jumping, run the drag function twice - this.points.drag() - this.points.drag() + for (let i = 0; i < this.DRAG_ITERATIONS; i++) { + this.points.drag() + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
logo.svgis excluded by!**/*.svgpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (82)
.eslintrc(1 hunks).github/SECURITY.md(1 hunks).github/workflows/github_pages.yml(1 hunks).gitignore(1 hunks).storybook/main.ts(1 hunks).storybook/manager-head.html(1 hunks).storybook/manager.ts(1 hunks).storybook/preview.ts(1 hunks).storybook/style.css(1 hunks)CITATION.cff(1 hunks)CODE_OF_CONDUCT.md(1 hunks)CONTRIBUTING.md(1 hunks)LICENCE(1 hunks)README.md(2 hunks)cosmos-2-0-migration-notes.md(1 hunks)package.json(3 hunks)rollup.config.js(2 hunks)src/attribution.ts(1 hunks)src/config.ts(8 hunks)src/declaration.d.ts(1 hunks)src/helper.ts(2 hunks)src/index.ts(19 hunks)src/modules/Clusters/calculate-centermass.vert(1 hunks)src/modules/Clusters/force-cluster.frag(1 hunks)src/modules/Clusters/index.ts(1 hunks)src/modules/Drag/index.ts(1 hunks)src/modules/ForceCenter/calculate-centermass.vert(1 hunks)src/modules/ForceCenter/force-center.frag(1 hunks)src/modules/ForceCenter/index.ts(1 hunks)src/modules/ForceGravity/force-gravity.frag(1 hunks)src/modules/ForceGravity/index.ts(1 hunks)src/modules/ForceLink/force-spring.ts(2 hunks)src/modules/ForceLink/index.ts(2 hunks)src/modules/ForceManyBody/calculate-level.vert(1 hunks)src/modules/ForceManyBody/force-centermass.frag(2 hunks)src/modules/ForceManyBody/force-level.frag(5 hunks)src/modules/ForceManyBody/index.ts(3 hunks)src/modules/ForceManyBody/quadtree-frag-shader.ts(2 hunks)src/modules/ForceManyBodyQuadtree/calculate-level.vert(1 hunks)src/modules/ForceManyBodyQuadtree/index.ts(1 hunks)src/modules/ForceManyBodyQuadtree/quadtree-frag-shader.ts(3 hunks)src/modules/ForceMouse/force-mouse.frag(1 hunks)src/modules/ForceMouse/index.ts(1 hunks)src/modules/GraphData/index.ts(1 hunks)src/modules/Lines/draw-curve-line.vert(4 hunks)src/modules/Lines/index.ts(1 hunks)src/modules/Points/color-buffer.ts(0 hunks)src/modules/Points/drag-point.frag(1 hunks)src/modules/Points/draw-highlighted.frag(1 hunks)src/modules/Points/draw-highlighted.vert(1 hunks)src/modules/Points/draw-points.frag(1 hunks)src/modules/Points/draw-points.vert(1 hunks)src/modules/Points/fill-sampled-points.frag(1 hunks)src/modules/Points/fill-sampled-points.vert(1 hunks)src/modules/Points/find-hovered-point.vert(2 hunks)src/modules/Points/find-points-on-area-selection.frag(1 hunks)src/modules/Points/index.ts(6 hunks)src/modules/Points/size-buffer.ts(0 hunks)src/modules/Points/track-positions.frag(1 hunks)src/modules/Points/tracked-buffer.ts(0 hunks)src/modules/Points/update-position.frag(1 hunks)src/modules/Shared/buffer.ts(2 hunks)src/modules/Shared/clear.frag(0 hunks)src/modules/Shared/quad.vert(1 hunks)src/modules/Store/index.ts(3 hunks)src/modules/Zoom/index.ts(4 hunks)src/modules/core-module.ts(1 hunks)src/stories/1. welcome.mdx(1 hunks)src/stories/2. configuration.mdx(1 hunks)src/stories/3. api-reference.mdx(1 hunks)src/stories/clusters.stories.ts(1 hunks)src/stories/clusters/radial.ts(1 hunks)src/stories/clusters/with-labels.ts(1 hunks)src/stories/clusters/worm.ts(1 hunks)src/stories/create-cluster-labels.ts(1 hunks)src/stories/create-cosmos.ts(1 hunks)src/stories/experiments.stories.ts(1 hunks)src/stories/experiments/full-mesh.ts(1 hunks)src/stories/experiments/mesh-with-holes.ts(1 hunks)src/stories/generate-mesh-data.ts(1 hunks)src/types.ts(0 hunks)src/variables.ts(1 hunks)
⛔ Files not processed due to max files limit (2)
- tsconfig.json
- vite.config.ts
💤 Files with no reviewable changes (5)
- src/modules/Shared/clear.frag
- src/modules/Points/tracked-buffer.ts
- src/types.ts
- src/modules/Points/size-buffer.ts
- src/modules/Points/color-buffer.ts
🚧 Files skipped from review as they are similar to previous changes (46)
- src/declaration.d.ts
- .gitignore
- .storybook/style.css
- .eslintrc
- src/modules/Points/fill-sampled-points.frag
- .storybook/manager.ts
- src/stories/clusters/radial.ts
- src/stories/clusters/worm.ts
- LICENCE
- src/attribution.ts
- src/modules/Points/draw-highlighted.frag
- src/stories/experiments/full-mesh.ts
- src/modules/ForceCenter/calculate-centermass.vert
- src/stories/experiments/mesh-with-holes.ts
- CONTRIBUTING.md
- src/modules/ForceManyBody/calculate-level.vert
- src/modules/ForceMouse/force-mouse.frag
- .storybook/main.ts
- src/modules/Points/update-position.frag
- src/stories/clusters/with-labels.ts
- src/modules/Points/draw-points.frag
- src/modules/ForceGravity/force-gravity.frag
- src/modules/Shared/quad.vert
- src/modules/ForceManyBodyQuadtree/calculate-level.vert
- src/modules/Points/track-positions.frag
- src/modules/Points/fill-sampled-points.vert
- .storybook/manager-head.html
- rollup.config.js
- CITATION.cff
- src/stories/experiments.stories.ts
- src/modules/ForceManyBody/force-level.frag
- .github/workflows/github_pages.yml
- src/modules/Lines/draw-curve-line.vert
- src/modules/Points/drag-point.frag
- src/modules/ForceCenter/force-center.frag
- src/modules/ForceManyBody/force-centermass.frag
- src/modules/Clusters/calculate-centermass.vert
- src/modules/ForceManyBody/quadtree-frag-shader.ts
- src/modules/Points/find-points-on-area-selection.frag
- src/modules/ForceManyBodyQuadtree/quadtree-frag-shader.ts
- src/modules/ForceLink/force-spring.ts
- src/stories/1. welcome.mdx
- .storybook/preview.ts
- src/stories/clusters.stories.ts
- src/modules/Points/draw-highlighted.vert
- src/modules/Drag/index.ts
🧰 Additional context used
🪛 LanguageTool
src/stories/2. configuration.mdx
[uncategorized] ~10-~10: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...applied only on component initialization and it can't be changed using the `setConfi...
(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~27-~27: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...ue, links are rendered as curved lines. Otherwise as straight lines | false | | curvedL...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[style] ~31-~31: Consider shortening this phrase to just ‘whether’, unless you mean ‘regardless of whether’.
Context: ... default link arrow value that controls whether or not to display link arrows | false | | li...
(WHETHER)
[uncategorized] ~35-~35: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...applied only on component initialization and it can't be changed using the `setConfi...
(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~38-~38: You might be missing the article “the” here.
Context: ...false | ## Simulation configuration Cosmos layout algorithm was inspired by the [d...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
README.md
[uncategorized] ~67-~67: Loose punctuation mark.
Context: ...aph.render() ``` - pointPositions: A Float32Array of `[x1, y1, x2, y2, ......
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~68-~68: Loose punctuation mark.
Context: ...y1, x2, y2, ..., xN, yN]. - **links**: A Float32Array of [sourceIndex1, targe...
(UNLIKELY_OPENING_PUNCTUATION)
src/stories/3. api-reference.mdx
[uncategorized] ~89-~89: Loose punctuation mark.
Context: ...link. Here’s a breakdown: * r: Red value (0.0-1.0) * g: Gree...
(UNLIKELY_OPENING_PUNCTUATION)
[duplication] ~296-~296: Possible typo: you repeated a word
Context: ...nPosition(coordinates) This method method is used to convert X and Y point coordi...
(ENGLISH_WORD_REPEAT_RULE)
[style] ~333-~333: ‘in conjunction with’ might be wordy. Consider a shorter alternative.
Context: ...uring the simulation. Using this method in conjunction with the [trackPointPositionsByIndices](...
(EN_WORDINESS_PREMIUM_IN_CONJUNCTION_WITH)
.github/SECURITY.md
[style] ~9-~9: Consider using a different verb for a more formal wording.
Context: ... This gives us time to work with you to fix the issue before public exposure, reduc...
(FIX_RESOLVE)
CODE_OF_CONDUCT.md
[uncategorized] ~3-~3: You might be missing the article “a” here.
Context: # Code of Conduct Cosmos, as member project of the OpenJS Foundation, use [...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~3-~3: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...ember project of the OpenJS Foundation, use [Contributor Covenant v2.0](https://con...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[typographical] ~19-~19: Consider adding a comma here.
Context: ...rting issues in spaces related to Cosmos please use the email [email protected]. Co...
(PLEASE_COMMA)
[style] ~23-~23: ‘with regard to’ might be wordy. Consider a shorter alternative.
Context: ...mit to: - maintain the confidentiality with regard to the reporter of an incident - to partic...
(EN_WORDINESS_PREMIUM_WITH_REGARD_TO)
[uncategorized] ~30-~30: Do not mix variants of the same word (‘organization’ and ‘organisation’) within a single text.
Context: ...example, repositories within the OpenJS organization, use the email `[email protected]...
(EN_WORD_COHERENCY)
[style] ~34-~34: ‘with regard to’ might be wordy. Consider a shorter alternative.
Context: ...its to: - maintain the confidentiality with regard to the reporter of an incident - to partic...
(EN_WORDINESS_PREMIUM_WITH_REGARD_TO)
[style] ~42-~42: Consider a shorter alternative to avoid wordiness.
Context: ... the CPC has not been properly handled. In order to escalate to the CoCP send an email to `...
(IN_ORDER_TO_PREMIUM)
[style] ~43-~43: Consider using “email”.
Context: ...ndled. In order to escalate to the CoCP send an email to [email protected]. Fo...
(SEND_AN_EMAIL)
[style] ~82-~82: Try using a synonym here to strengthen your wording.
Context: ...ind * Trolling, insulting or derogatory comments, and personal or political attacks * Pu...
(COMMENT_REMARK)
[uncategorized] ~171-~171: You might be missing the article “the” here.
Context: ...nant.org/version/2/0/code_of_conduct). Community Impact Guidelines were inspired by [Moz...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🪛 Markdownlint (0.37.0)
README.md
118-118: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
cosmos-2-0-migration-notes.md
77-77: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
81-81: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
87-87: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
93-93: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
CODE_OF_CONDUCT.md
70-70: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
71-71: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
72-72: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
73-73: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
75-75: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
80-80: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
82-82: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
83-83: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
84-84: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
86-86: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
6-6: null
Link fragments should be valid
(MD051, link-fragments)
🔇 Additional comments (26)
src/stories/create-cluster-labels.ts (1)
1-4: LGTM! Clean function signature and initialization.
The closure pattern is well-implemented, providing a clean way to maintain the label divs state.
CODE_OF_CONDUCT.md (1)
1-178: Overall, this is a well-structured Code of Conduct implementation
The document successfully establishes a comprehensive Code of Conduct for the Cosmos project, with clear reporting procedures and enforcement guidelines. The suggested changes are primarily formatting and style improvements that will enhance readability without altering the core content.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: You might be missing the article “a” here.
Context: # Code of Conduct Cosmos, as member project of the OpenJS Foundation, use [...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~3-~3: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...ember project of the OpenJS Foundation, use [Contributor Covenant v2.0](https://con...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[typographical] ~19-~19: Consider adding a comma here.
Context: ...rting issues in spaces related to Cosmos please use the email [email protected]. Co...
(PLEASE_COMMA)
[style] ~23-~23: ‘with regard to’ might be wordy. Consider a shorter alternative.
Context: ...mit to: - maintain the confidentiality with regard to the reporter of an incident - to partic...
(EN_WORDINESS_PREMIUM_WITH_REGARD_TO)
[uncategorized] ~30-~30: Do not mix variants of the same word (‘organization’ and ‘organisation’) within a single text.
Context: ...example, repositories within the OpenJS organization, use the email `[email protected]...
(EN_WORD_COHERENCY)
[style] ~34-~34: ‘with regard to’ might be wordy. Consider a shorter alternative.
Context: ...its to: - maintain the confidentiality with regard to the reporter of an incident - to partic...
(EN_WORDINESS_PREMIUM_WITH_REGARD_TO)
[style] ~42-~42: Consider a shorter alternative to avoid wordiness.
Context: ... the CPC has not been properly handled. In order to escalate to the CoCP send an email to `...
(IN_ORDER_TO_PREMIUM)
[style] ~43-~43: Consider using “email”.
Context: ...ndled. In order to escalate to the CoCP send an email to [email protected]. Fo...
(SEND_AN_EMAIL)
[style] ~82-~82: Try using a synonym here to strengthen your wording.
Context: ...ind * Trolling, insulting or derogatory comments, and personal or political attacks * Pu...
(COMMENT_REMARK)
[uncategorized] ~171-~171: You might be missing the article “the” here.
Context: ...nant.org/version/2/0/code_of_conduct). Community Impact Guidelines were inspired by [Moz...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🪛 Markdownlint (0.37.0)
70-70: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
71-71: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
72-72: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
73-73: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
75-75: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
80-80: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
82-82: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
83-83: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
84-84: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
86-86: Expected: dash; Actual: asterisk
Unordered list style
(MD004, ul-style)
6-6: null
Link fragments should be valid
(MD051, link-fragments)
src/stories/generate-mesh-data.ts (1)
107-121: LGTM!
The return statement correctly matches the MeshData type definition and properly converts the links array to Float32Array.
.github/SECURITY.md (1)
9-11: 🛠️ Refactor suggestion
Improve vulnerability reporting structure
The reporting instructions could be more formal and structured.
-If you have discovered a security vulnerability in this project, please report it privately. **Do not disclose it as a public issue.** This gives us time to work with you to fix the issue before public exposure, reducing the chance that the exploit will be used before a patch is released.
-
-Please disclose it at [security advisory](https://github.com/cosmograph-org/cosmos/security/advisories/new).
+## Reporting Process
+
+1. **Do not** create a public issue for security vulnerabilities
+2. Submit your report through our [Security Advisory Portal](https://github.com/cosmograph-org/cosmos/security/advisories/new)
+3. Include in your report:
+ - Detailed description of the vulnerability
+ - Steps to reproduce
+ - Potential impact assessment
+ - Suggested remediation if availableLikely invalid or redundant comment.
🧰 Tools
🪛 LanguageTool
[style] ~9-~9: Consider using a different verb for a more formal wording.
Context: ... This gives us time to work with you to fix the issue before public exposure, reduc...
(FIX_RESOLVE)
src/modules/core-module.ts (1)
7-12: Verify impact of generic type removal
The removal of generic types N and L is a breaking change that affects type safety. While this simplifies the API, it requires careful verification of type handling throughout the codebase.
✅ Verification successful
Generic type removal is part of documented API changes
The removal of generic types from CoreModule is a deliberate part of the v2.0 migration, which transitions from a node-centric to a point-centric model. Based on the migration notes and codebase analysis:
- All module implementations (ForceLink, Points, Lines, etc.) are already using the non-generic CoreModule
- The change is well-documented in the migration guide, which explicitly mentions the shift from node objects to point indices
- No existing code in the codebase relies on the generic types, as evidenced by the absence of
CoreModule<usage - The migration notes clearly explain the new data handling approach using Float32Arrays instead of generic node objects
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing generic type usage that might be affected
rg -l "CoreModule<"
# Check for potential type casting in consuming code
rg "as \w+Node|as \w+Link"
# Look for custom type extensions that might break
ast-grep --pattern 'interface $_ extends CosmosInputNode'
Length of output: 110
Script:
#!/bin/bash
# Let's try a broader search to understand the usage patterns and dependencies
# Search for files importing or using CoreModule
rg -l "CoreModule"
# Look for any interfaces or types that might be affected by the generic removal
rg "interface.*Node|type.*Node" -A 2
# Check for any type parameters or generic usage in related files
ast-grep --pattern 'class $_ <$_>'
# Search for files containing both Node and Link type references
rg -l "(?s).*Node.*Link|.*Link.*Node"
Length of output: 484
Script:
#!/bin/bash
# Let's examine the actual usage of CoreModule in the dependent files
# Check how CoreModule is used in force and visualization modules
rg "CoreModule" -B 2 -A 2 src/modules/Force*/index.ts src/modules/Points/index.ts src/modules/Lines/index.ts
# Look at the migration notes for context about the breaking change
cat cosmos-2-0-migration-notes.md
# Check for any type definitions in the current implementation
rg "type|interface" src/modules/core-module.ts
Length of output: 9234
src/modules/ForceMouse/index.ts (2)
7-7: LGTM: Generic type parameters removal
The simplified class signature aligns with the project-wide transition to a non-generic structure, improving code maintainability.
12-26: Verify the framebuffer casting safety
The initialization looks good with improved naming conventions. However, the framebuffer casting needs verification.
✅ Verification successful
Based on the search results, I can now generate the final response:
Framebuffer casting pattern is consistently implemented and safe
The type casting of points?.velocityFbo as regl.Framebuffer2D is consistently used across the codebase with proper null checks through the optional chaining operator (?.). This pattern is used in all force-related modules (ForceCenter, ForceMouse, ForceManyBody, etc.) and follows the same safety practices:
- Optional chaining is used before accessing framebuffers
- Type casting is consistently applied
- Framebuffers are accessed within command definitions where regl handles the execution context
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential type safety issues with framebuffer casting
# Look for similar patterns across the codebase to ensure consistency
# Search for framebuffer type casting patterns
rg "as regl\.Framebuffer2D" -A 2 -B 2
# Search for potential null checks before framebuffer access
rg "points\?\." -A 2 -B 2
Length of output: 19342
src/modules/ForceGravity/index.ts (1)
7-7: LGTM: Consistent with ForceMouse changes
The simplified class signature follows the same pattern as ForceMouse, maintaining consistency across modules.
src/modules/Clusters/force-cluster.frag (2)
5-12: LGTM: Well-organized uniform declarations
The uniform declarations are clearly organized and properly typed, following GLSL best practices.
30-35: Efficient vector operations implementation
The implementation already uses the optimized approach with normalize() instead of trigonometric functions, which was suggested in a previous review.
src/variables.ts (1)
30-41: Verify simulation parameter changes
The simulation parameters have been significantly adjusted:
- decay: 1000 → 5000
- gravity: 0 → 0.25
- repulsion: 0.1 → 1.0
- linkDistance: 2 → 10
- cluster: 0.1 (new)
These changes could significantly impact the graph layout behavior and performance.
Let's check if these parameters are documented:
✅ Verification successful
The previous search might have been too specific. Let's broaden our search to:
- Look for any mentions of these specific parameters in markdown files
- Check all README files for documentation
- Look for comments near the parameter definitions in the code
Parameter changes are documented and aligned with the public API
The parameter changes are valid and documented. From the search results:
-
The parameters have corresponding public API configurations in a different naming format:
simulationFrictionsimulationGravitysimulationRepulsion- etc.
-
The README shows these parameters are part of the public configuration API, with examples demonstrating their usage:
const config = {
simulationFriction: 0.1,
simulationGravity: 0,
simulationRepulsion: 0.5,
/* ... */
}- The implementation in the codebase consistently uses these parameters across different force calculation modules:
ForceGravity/index.tsusessimulationGravityForceManyBody/index.tsusessimulationRepulsionForceLink/index.tsusessimulationLinkDistance
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for documentation about simulation parameters
rg -i "simulation parameters|simulation config" --type md
Length of output: 59
Script:
#!/bin/bash
# Let's try a broader search for documentation about these parameters
rg -i "(decay|gravity|repulsion|linkdistance|cluster)\s*(parameter|config|setting)" --type md
# Also search for any README files that might contain documentation
fd "README" --extension md --exec cat {}
# Look for comments near these parameter definitions
rg -B 5 -A 5 "decay:|gravity:|repulsion:|linkDistance:|cluster:" --type ts
Length of output: 11092
src/modules/Points/draw-points.vert (3)
50-57: Improved color and alpha handling
The color and alpha handling has been simplified and improved:
- Direct use of vec4 color attribute
- Cleaner alpha modification for greyed-out points
42-46: Verify transformation matrix usage
The point position transformation has been updated to use normalized device coordinates with the transformation matrix. This is a critical change that affects all point positioning.
Let's check for consistency in transformation handling:
✅ Verification successful
The previous command failed because the glsl file type wasn't recognized. Let's try a different approach by first identifying shader files and then searching for transformation matrix usage across all files, as shader files might not have a recognized file type association.
I want to check the implementation details of the vertex shaders to verify the transformation matrix usage consistency, particularly in the point and line drawing shaders since they both reference the transformation matrix in their respective index files.
Transformation matrix usage is consistent across shaders
After examining the shader implementations, the transformation matrix usage in draw-points.vert is consistent with other shaders:
- All shaders (
draw-points.vert,draw-highlighted.vert, anddraw-curve-line.vert) follow the same pattern:- Convert positions to normalized device coordinates
- Apply screen size adjustment
- Transform using
transformationMatrixwith homogeneous coordinates (vec3) - Output final position as vec4 with z=0 and w=1
The updated transformation in draw-points.vert matches this established pattern, ensuring consistent point positioning across the visualization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for transformation matrix usage in other shaders
rg "transformationMatrix|transform.*mat3" --type glsl
Length of output: 84
Script:
#!/bin/bash
# Search for files with .vert or .frag extensions (common shader file extensions)
fd "\.(vert|frag)$"
# Then search for transformation matrix usage in these files
rg "transformationMatrix|transform.*mat3" -t all
Length of output: 1920
Script:
#!/bin/bash
# Let's examine the content of draw-points.vert and other related vertex shaders
cat src/modules/Points/draw-points.vert
cat src/modules/Points/draw-highlighted.vert
cat src/modules/Lines/draw-curve-line.vert
Length of output: 7498
25-33: Review point size calculation changes
The calculatePointSize function has been modified to use transformationMatrix instead of a direct transform. The scaling behavior is now controlled by scalePointsOnZoom.
Let's check for potential issues with this approach:
✅ Verification successful
The previous command failed because the file type 'glsl' wasn't recognized. Let's try searching by file extensions and content patterns instead, which should help us find shader files and point size calculations across the codebase.
The previous search revealed several shader files. Let's examine the content of the point-drawing shaders and also check how these parameters are configured in the JavaScript code to ensure the new point size calculation approach is consistent with the rest of the codebase.
The shader files show that both draw-points.vert and draw-highlighted.vert have similar point size calculation logic using transformationMatrix and scalePointsOnZoom. Let's check how these uniforms are set in the JavaScript code to ensure the implementation is consistent with the application's requirements.
Based on the examination of both shader files, I can now generate the final response:
Point size calculation implementation is consistent across shaders ✓
The modified calculatePointSize function in draw-points.vert follows the same approach as the one in draw-highlighted.vert, with both implementing:
- Conditional scaling based on
scalePointsOnZoom - Proper use of
transformationMatrix[0][0]for zoom-based scaling - Consistent maximum point size capping
The only minor difference is that draw-points.vert includes an additional ratio factor, which is appropriate as it handles the base point rendering while draw-highlighted.vert is specifically for highlight effects.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related point size calculations in other shaders
rg "pointSize|calculatePointSize" --type glsl
Length of output: 76
Script:
#!/bin/bash
# Search for shader files and point size calculations using file extensions and content
rg -t vert -t frag -t glsl "pointSize|calculatePointSize|gl_PointSize"
# Also search for files with shader extensions
fd "\.(vert|frag|glsl)$"
Length of output: 1417
Script:
#!/bin/bash
# Let's check the content of relevant point-drawing shaders
cat src/modules/Points/draw-points.vert src/modules/Points/draw-highlighted.vert
# Also check for any JavaScript files that might configure these shader parameters
rg "scalePointsOnZoom|transformationMatrix|maxPointSize" --type js
Length of output: 3731
Script:
#!/bin/bash
# Search for JavaScript files that set these uniforms
rg -t js "\.setUniform\(" -A 1 -B 1
Length of output: 37
src/modules/Points/find-hovered-point.vert (1)
5-18: Verify uniform name changes across all shaders
The renaming of uniforms (e.g., positionsTexture, transformationMatrix, scalePointsOnZoom) improves clarity but requires consistent updates across all shader files.
✅ Verification successful
The previous script failed because the .glsl file type wasn't recognized. Let's try with explicit file extensions and add them to ripgrep's type definitions. This will help us verify the uniform naming consistency across all shader files.
Uniform naming is consistent across shader files
Based on the comprehensive search results, the uniform naming in find-hovered-point.vert is consistent with other shader files in the codebase. Specifically:
positionsTextureis used consistently across all shaders that need position datatransformationMatrixis used consistently in vertex shaders for transformationscalePointsOnZoomis used consistently in point-related shaders (e.g.,draw-points.vert,find-points-on-area-selection.frag)- Other uniforms like
pointsTextureSize,sizeScale,spaceSize,screenSize,ratio, andmaxPointSizeare also consistently named across related shaders
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistency in uniform naming across shaders
echo "Checking uniform naming consistency across shaders:"
rg -t glsl "uniform\s+(sampler2D|mat3|bool)\s+(position|transform|scaleNodesOnZoom)" \
-A 1 -B 1
Length of output: 238
Script:
#!/bin/bash
# Search for shader files and their uniform declarations
echo "Searching for shader files:"
fd "\.(vert|frag|glsl)$"
echo -e "\nChecking uniform declarations in shader files:"
rg "uniform\s+" --type-add 'shader:*.{vert,frag,glsl}' -t shader -A 1 -B 1
Length of output: 12561
package.json (3)
3-3: Major version and license changes require careful consideration
The changes to version (2.0.0-alpha.8) and license (MIT) are significant:
- Alpha version indicates breaking changes are still possible
- License change from CC-BY-NC-4.0 to MIT affects usage terms
Also applies to: 8-8
72-72: Update TypeScript to latest stable version
TypeScript v5.5.2 is currently in beta. Consider updating to the latest stable version.
13-18: Verify build output consistency between rollup and vite
The addition of Vite alongside Rollup requires verification that both build systems produce consistent output.
✅ Verification successful
Let me gather more information about the build outputs to verify consistency.
Let me analyze the build outputs to verify consistency.
Build configurations are consistent between Rollup and Vite
The build configurations show proper alignment:
- Both produce ES and UMD formats
- Both generate source maps
- Both handle minification (Rollup via terser plugin, Vite built-in)
- Both properly externalize dependencies
- Output files are consistent:
- ES:
dist/index.js - UMD (minified):
dist/index.min.js
- ES:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential conflicts in build configurations
echo "Checking build configurations:"
fd -e js -e ts "vite\.config\.|rollup\.config\." \
--exec cat {} \;
Length of output: 3240
Script:
#!/bin/bash
# Check build output file patterns and configurations
echo "Checking package.json build scripts:"
rg "\"build.*\":|\"watch.*\":" package.json -A 1
echo -e "\nChecking source entry point:"
cat src/index.ts
echo -e "\nChecking for type definitions:"
fd -e d.ts "index\.d\.ts$" --exec cat {} \;
Length of output: 45504
Script:
#!/bin/bash
# Compare build configurations and outputs
echo "Analyzing build configurations:"
# Check output file patterns
echo "Output file patterns:"
rg "file.*:|fileName.*:" rollup.config.* vite.config.*
# Check format configurations
echo -e "\nOutput formats:"
rg "format.*:|formats.*:" rollup.config.* vite.config.*
# Check external dependencies
echo -e "\nExternal dependencies:"
rg "external.*:|externals.*:" rollup.config.* vite.config.*
# Check source maps and minification
echo -e "\nSource maps and minification:"
rg "sourcemap.*:|minify.*:" rollup.config.* vite.config.*
Length of output: 1327
src/modules/ForceCenter/index.ts (1)
56-56: Add validation for pointsNumber
The count property uses data.pointsNumber ?? 0 which could result in zero vertices being rendered if pointsNumber is undefined.
src/modules/Zoom/index.ts (3)
8-10: LGTM: Type system simplification
The removal of generic type parameters aligns with the codebase-wide simplification effort.
Also applies to: 44-47
67-75: Excellent edge case handling!
The addition of extent adjustment logic prevents division by zero when points share the same coordinates. This is a crucial fix for maintaining visualization stability.
139-141: LGTM: Consistent terminology
The property rename from scaleNodesOnZoom to scalePointsOnZoom maintains consistency with the new point-centric terminology.
src/modules/ForceManyBody/index.ts (2)
85-85: Add validation for point count in calculateLevelsCommand
The count property uses data.pointsNumber which could be undefined. Consider adding validation or a fallback value.
7-7:
Update import statement for consistency
The import statement has been updated from createIndexesBuffer to createIndexesForBuffer, but the old function name is still used in line 63.
Apply this diff to fix the inconsistency:
- this.pointIndices(createIndexesForBuffer(store.pointsTextureSize))
+ this.pointIndices(createIndexesBuffer(store.pointsTextureSize))Likely invalid or redundant comment.
src/modules/Points/index.ts (1)
466-473: Well-handled division by zero protection
Good practice using defaultConfigValues.pointSamplingDistance as fallback when dist is zero.
src/index.ts (1)
1107-1124: 🛠️ Refactor suggestion
Enhance attribution element security
The current implementation could benefit from additional security measures when handling SVG content.
Apply these security enhancements:
private addAttribution (): void {
if (this.config.disableAttribution) return
this.attributionDivElement = document.createElement('div')
+ // Sanitize styles to prevent CSS injection
+ const safeStyles = [
+ 'cursor: pointer',
+ 'user-select: none',
+ 'position: absolute',
+ 'margin: 0 0.6rem 0.6rem 0',
+ 'line-height: 0',
+ 'bottom: 0',
+ 'right: 0',
+ 'color: var(--cosmos-attribution-color)'
+ ].join(';')
- this.attributionDivElement.style.cssText = `
- cursor: pointer;
- user-select: none;
- position: absolute;
- margin: 0 0.6rem 0.6rem 0;
- line-height: 0;
- bottom: 0;
- right: 0;
- color: var(--cosmos-attribution-color);
- `
+ this.attributionDivElement.style.cssText = safeStyles
this.attributionDivElement.onclick = (): void => window.open('https://cosmograph.app/', '_blank')?.focus()
const svgParser = new DOMParser()
+ // Validate SVG content before parsing
+ if (!attributionSvg.startsWith('<svg') || !attributionSvg.endsWith('</svg>')) {
+ console.warn('Invalid SVG content in attribution')
+ return
+ }
const svgElement = svgParser.parseFromString(attributionSvg, 'image/svg+xml').firstChild as SVGElement
+ if (!(svgElement instanceof SVGElement)) {
+ console.warn('Failed to parse attribution SVG')
+ return
+ }
this.attributionDivElement.appendChild(svgElement)
this.store.div?.appendChild(this.attributionDivElement)
}Likely invalid or redundant comment.
| if (clusterLabelDivs.length === 0) { | ||
| for (let i = 0; i < nClusters; i++) { | ||
| const clusterLabelDiv = document.createElement('div') | ||
| const contentLabel = document.createElement('p') | ||
| clusterLabelDiv.appendChild(contentLabel) | ||
| clusterLabelDiv.style.position = 'absolute' | ||
| clusterLabelDiv.style.pointerEvents = 'none' | ||
|
|
||
| contentLabel.style.fontFamily = [ | ||
| '"Nunito Sans"', | ||
| '-apple-system', | ||
| '".SFNSText-Regular"', | ||
| '"San Francisco"', | ||
| 'BlinkMacSystemFont', | ||
| '"Segoe UI"', | ||
| '"Helvetica Neue"', | ||
| 'Helvetica', | ||
| 'Arial', | ||
| 'sans-serif', | ||
| ].join(', ') | ||
| contentLabel.style.fontWeight = 'bold' | ||
| contentLabel.style.color = 'white' | ||
| contentLabel.style.transform = 'translate(-50%, -100%)' | ||
| contentLabel.innerText = `Cluster ${i + 1}` | ||
|
|
||
| props.div.appendChild(clusterLabelDiv) | ||
| clusterLabelDivs[i] = clusterLabelDiv | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and add cleanup mechanism.
Consider the following improvements:
- Add ARIA attributes for accessibility
- Implement a cleanup mechanism to remove old labels when the component is destroyed
if (clusterLabelDivs.length === 0) {
for (let i = 0; i < nClusters; i++) {
const clusterLabelDiv = document.createElement('div')
const contentLabel = document.createElement('p')
+ clusterLabelDiv.setAttribute('role', 'tooltip')
+ clusterLabelDiv.setAttribute('aria-label', `Cluster ${i + 1}`)
clusterLabelDiv.appendChild(contentLabel)Also, consider adding a cleanup function:
function cleanup(): void {
clusterLabelDivs.forEach(div => div.remove())
clusterLabelDivs.length = 0
}| Cosmos, as member project of the OpenJS Foundation, use [Contributor Covenant | ||
| v2.0](https://contributor-covenant.org/version/2/0/code_of_conduct) as their | ||
| code of conduct. The full text is included | ||
| [below](#contributor-covenant-code-of-conduct) in English, and translations are |
There was a problem hiding this comment.
Fix grammar and link fragment issues in the introduction
There are a few issues that need to be addressed:
- Grammar in line 3: "Cosmos, as member project" should be "Cosmos, as a member project"
- Verb agreement in line 3: "use" should be "uses"
- The link fragment
#contributor-covenant-code-of-conductdoesn't match any heading in the document
Apply this diff to fix the issues:
-Cosmos, as member project of the OpenJS Foundation, use [Contributor Covenant
+Cosmos, as a member project of the OpenJS Foundation, uses [Contributor Covenant
v2.0](https://contributor-covenant.org/version/2/0/code_of_conduct) as their
code of conduct. The full text is included
-[below](#contributor-covenant-code-of-conduct) in English, and translations are
+[below](#contributor-covenant-code-of-conduct-v20) in English, and translations are📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Cosmos, as member project of the OpenJS Foundation, use [Contributor Covenant | |
| v2.0](https://contributor-covenant.org/version/2/0/code_of_conduct) as their | |
| code of conduct. The full text is included | |
| [below](#contributor-covenant-code-of-conduct) in English, and translations are | |
| Cosmos, as a member project of the OpenJS Foundation, uses [Contributor Covenant | |
| v2.0](https://contributor-covenant.org/version/2/0/code_of_conduct) as their | |
| code of conduct. The full text is included | |
| [below](#contributor-covenant-code-of-conduct-v20) in English, and translations are |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: You might be missing the article “a” here.
Context: # Code of Conduct Cosmos, as member project of the OpenJS Foundation, use [...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~3-~3: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...ember project of the OpenJS Foundation, use [Contributor Covenant v2.0](https://con...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
🪛 Markdownlint (0.37.0)
6-6: null
Link fragments should be valid
(MD051, link-fragments)
| export type CosmosStoryProps = GraphConfigInterface & { | ||
| pointPositions: Float32Array; | ||
| pointColors: Float32Array; | ||
| pointSizes?: Float32Array; | ||
|
|
||
| links?: Float32Array; | ||
| linkColors?: Float32Array; | ||
| linkWidths?: Float32Array; | ||
| // linkStrength?: Float32Array; | ||
|
|
||
| pointClusters?: number[]; | ||
| clusterPositions?: number[]; | ||
| clusterStrength?: Float32Array; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Uncomment and implement the linkStrength property as it's actively used in the codebase
The commented-out linkStrength property in CosmosStoryProps should be uncommented as:
- It's a supported feature in the core library (
src/index.tshassetLinkStrengthmethod) - It's actively used in force calculations (
src/modules/ForceLink/index.ts) - The implementation is already partially done (commented code in
create-cosmos.tsshowsgraph.setLinkStrength(props.linkStrength))
🔗 Analysis chain
LGTM! Well-structured type definition with performance considerations.
The use of Float32Array for positions, colors, and other numerical data is a good choice for performance-critical visualization data.
Let's verify if the commented out linkStrength property is used elsewhere:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if linkStrength is used in other files
rg "linkStrength" --type ts
Length of output: 1221
| const graph = new Graph(div, config) | ||
|
|
||
| graph.setPointPositions(props.pointPositions) | ||
| graph.setPointColors(props.pointColors) | ||
| if (props.pointSizes) graph.setPointSizes(props.pointSizes) | ||
|
|
||
| if (props.links) graph.setLinks(props.links) | ||
| if (props.linkColors) graph.setLinkColors(props.linkColors) | ||
| if (props.linkWidths) graph.setLinkWidths(props.linkWidths) | ||
| // if (props.linkStrength) graph.setLinkStrength(props.linkStrength) | ||
|
|
||
| if (props.pointClusters) graph.setPointClusters(props.pointClusters) | ||
| if (props.clusterPositions) graph.setClusterPositions(props.clusterPositions) | ||
| if (props.clusterStrength) graph.setPointClusterStrength(props.clusterStrength) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for invalid data.
While the code handles optional properties well, it might benefit from validation of the mandatory properties and data consistency checks.
Consider adding validation:
const graph = new Graph(div, config)
+ // Validate mandatory properties
+ if (props.pointPositions.length === 0) {
+ throw new Error('Point positions array cannot be empty')
+ }
+ if (props.pointColors.length !== props.pointPositions.length / 2) {
+ throw new Error('Colors array length must match number of points')
+ }
graph.setPointPositions(props.pointPositions)
graph.setPointColors(props.pointColors)Also, remove the commented linkStrength setter if it's no longer needed:
if (props.links) graph.setLinks(props.links)
if (props.linkColors) graph.setLinkColors(props.linkColors)
if (props.linkWidths) graph.setLinkWidths(props.linkWidths)
- // if (props.linkStrength) graph.setLinkStrength(props.linkStrength)Committable suggestion skipped: line range outside the PR's diff.
| vec2 clusterPositions = texture2D(clusterPositionsTexture, pointClusterIndices.xy / clustersTextureSize).xy; | ||
| if (clusterPositions.x < 0.0 || clusterPositions.y < 0.0) { | ||
| vec4 centermassValues = texture2D(centermassTexture, pointClusterIndices.xy / clustersTextureSize); | ||
| clusterPositions = centermassValues.xy / centermassValues.b; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding bounds checking for texture coordinates
When accessing centermassTexture, ensure the texture coordinates are within valid bounds to prevent undefined behavior.
vec2 clusterPositions = texture2D(clusterPositionsTexture, pointClusterIndices.xy / clustersTextureSize).xy;
if (clusterPositions.x < 0.0 || clusterPositions.y < 0.0) {
+ vec2 texCoord = pointClusterIndices.xy / clustersTextureSize;
+ if (texCoord.x >= 0.0 && texCoord.x <= 1.0 && texCoord.y >= 0.0 && texCoord.y <= 1.0) {
vec4 centermassValues = texture2D(centermassTexture, pointClusterIndices.xy / clustersTextureSize);
clusterPositions = centermassValues.xy / centermassValues.b;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vec2 clusterPositions = texture2D(clusterPositionsTexture, pointClusterIndices.xy / clustersTextureSize).xy; | |
| if (clusterPositions.x < 0.0 || clusterPositions.y < 0.0) { | |
| vec4 centermassValues = texture2D(centermassTexture, pointClusterIndices.xy / clustersTextureSize); | |
| clusterPositions = centermassValues.xy / centermassValues.b; | |
| } | |
| vec2 clusterPositions = texture2D(clusterPositionsTexture, pointClusterIndices.xy / clustersTextureSize).xy; | |
| if (clusterPositions.x < 0.0 || clusterPositions.y < 0.0) { | |
| vec2 texCoord = pointClusterIndices.xy / clustersTextureSize; | |
| if (texCoord.x >= 0.0 && texCoord.x <= 1.0 && texCoord.y >= 0.0 && texCoord.y <= 1.0) { | |
| vec4 centermassValues = texture2D(centermassTexture, pointClusterIndices.xy / clustersTextureSize); | |
| clusterPositions = centermassValues.xy / centermassValues.b; | |
| } | |
| } |
| const grouped = direction === LinkDirection.INCOMING ? data.sourceIndexToTargetIndices : data.targetIndexToSourceIndices | ||
| this.maxPointDegree = 0 | ||
| let linkIndex = 0 | ||
| grouped.forEach((connectedNodeIndices, nodeIndex) => { | ||
| this.linkFirstIndicesAndAmount[nodeIndex * 4 + 0] = linkIndex % linksTextureSize | ||
| this.linkFirstIndicesAndAmount[nodeIndex * 4 + 1] = Math.floor(linkIndex / linksTextureSize) | ||
| this.linkFirstIndicesAndAmount[nodeIndex * 4 + 2] = connectedNodeIndices.size | ||
| grouped?.forEach((connectedPointIndices, pointIndex) => { | ||
| if (connectedPointIndices) { | ||
| this.linkFirstIndicesAndAmount[pointIndex * 4 + 0] = linkIndex % linksTextureSize | ||
| this.linkFirstIndicesAndAmount[pointIndex * 4 + 1] = Math.floor(linkIndex / linksTextureSize) | ||
| this.linkFirstIndicesAndAmount[pointIndex * 4 + 2] = connectedPointIndices.length ?? 0 | ||
|
|
||
| connectedNodeIndices.forEach((connectedNodeIndex) => { | ||
| this.indices[linkIndex * 4 + 0] = connectedNodeIndex % pointsTextureSize | ||
| this.indices[linkIndex * 4 + 1] = Math.floor(connectedNodeIndex / pointsTextureSize) | ||
| const degree = data.degree[data.getInputIndexBySortedIndex(connectedNodeIndex) as number] ?? 0 | ||
| const connectedDegree = data.degree[data.getInputIndexBySortedIndex(nodeIndex) as number] ?? 0 | ||
| const bias = degree / (degree + connectedDegree) | ||
| let strength = 1 / Math.min(degree, connectedDegree) | ||
| strength = Math.sqrt(strength) | ||
| linkBiasAndStrengthState[linkIndex * 4 + 0] = bias | ||
| linkBiasAndStrengthState[linkIndex * 4 + 1] = strength | ||
| linkDistanceState[linkIndex * 4] = this.store.getRandomFloat(0, 1) | ||
| connectedPointIndices.forEach(([connectedPointIndex, initialLinkIndex]) => { | ||
| this.indices[linkIndex * 4 + 0] = connectedPointIndex % pointsTextureSize | ||
| this.indices[linkIndex * 4 + 1] = Math.floor(connectedPointIndex / pointsTextureSize) | ||
| const degree = data.degree?.[connectedPointIndex] ?? 0 | ||
| const connectedDegree = data.degree?.[pointIndex] ?? 0 | ||
| const degreeSum = degree + connectedDegree | ||
| // Prevent division by zero | ||
| const bias = degreeSum !== 0 ? degree / degreeSum : 0.5 | ||
| const minDegree = Math.min(degree, connectedDegree) | ||
| // Prevent division by zero | ||
| let strength = data.linkStrength?.[initialLinkIndex] ?? (1 / Math.max(minDegree, 1)) | ||
| strength = Math.sqrt(strength) | ||
| linkBiasAndStrengthState[linkIndex * 4 + 0] = bias | ||
| linkBiasAndStrengthState[linkIndex * 4 + 1] = strength | ||
| linkDistanceState[linkIndex * 4] = this.store.getRandomFloat(0, 1) | ||
|
|
||
| linkIndex += 1 | ||
| }) | ||
| linkIndex += 1 | ||
| }) | ||
|
|
||
| this.maxPointDegree = Math.max(this.maxPointDegree, connectedNodeIndices.size) | ||
| this.maxPointDegree = Math.max(this.maxPointDegree, connectedPointIndices.length ?? 0) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for grouped indices
The method should validate the grouped indices structure to prevent potential runtime errors.
+ private validateGroupedIndices(
+ grouped: Map<number, [number, number][]> | undefined,
+ pointsTextureSize: number
+ ): boolean {
+ if (!grouped) return false
+ const maxValidIndex = pointsTextureSize * pointsTextureSize - 1
+ for (const [pointIndex, indices] of grouped.entries()) {
+ if (pointIndex < 0 || pointIndex > maxValidIndex) {
+ console.error(`Invalid point index: ${pointIndex}`)
+ return false
+ }
+ for (const [connectedIndex] of indices) {
+ if (connectedIndex < 0 || connectedIndex > maxValidIndex) {
+ console.error(`Invalid connected index: ${connectedIndex}`)
+ return false
+ }
+ }
+ }
+ return true
+ }
const grouped = direction === LinkDirection.INCOMING ? data.sourceIndexToTargetIndices : data.targetIndexToSourceIndices
+ if (!this.validateGroupedIndices(grouped, store.pointsTextureSize)) {
+ console.error('Invalid grouped indices detected')
+ return
+ }Committable suggestion skipped: line range outside the PR's diff.
| public run (): void { | ||
| if (!this.data.pointClusters && !this.data.clusterPositions) return | ||
| this.clearCentermassCommand?.() | ||
| this.calculateCentermassCommand?.() | ||
| this.applyForcesCommand?.() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add comprehensive error handling to run method
Enhance the run method with proper error handling and state validation.
public run(): void {
- if (!this.data.pointClusters && !this.data.clusterPositions) return
+ try {
+ if (!this.data.pointClusters && !this.data.clusterPositions) {
+ return
+ }
+
+ if (!this.centermassFbo || !this.clusterFbo || !this.clusterPositionsFbo) {
+ console.error('Required framebuffers not initialized')
+ return
+ }
+
+ if (!this.clearCentermassCommand || !this.calculateCentermassCommand || !this.applyForcesCommand) {
+ console.error('Required commands not initialized')
+ return
+ }
this.clearCentermassCommand()
this.calculateCentermassCommand()
this.applyForcesCommand()
+ } catch (error) {
+ console.error('Error during cluster force calculation:', error)
+ throw error
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public run (): void { | |
| if (!this.data.pointClusters && !this.data.clusterPositions) return | |
| this.clearCentermassCommand?.() | |
| this.calculateCentermassCommand?.() | |
| this.applyForcesCommand?.() | |
| } | |
| public run (): void { | |
| try { | |
| if (!this.data.pointClusters && !this.data.clusterPositions) { | |
| return | |
| } | |
| if (!this.centermassFbo || !this.clusterFbo || !this.clusterPositionsFbo) { | |
| console.error('Required framebuffers not initialized') | |
| return | |
| } | |
| if (!this.clearCentermassCommand || !this.calculateCentermassCommand || !this.applyForcesCommand) { | |
| console.error('Required commands not initialized') | |
| return | |
| } | |
| this.clearCentermassCommand() | |
| this.calculateCentermassCommand() | |
| this.applyForcesCommand() | |
| } catch (error) { | |
| console.error('Error during cluster force calculation:', error) | |
| throw error | |
| } | |
| } |
| public create (): void { | ||
| const { reglInstance, store, data } = this | ||
| const { pointsTextureSize } = store | ||
| if (data.pointsNumber === undefined || (!data.pointClusters && !data.clusterPositions)) return | ||
|
|
||
| // Find the highest cluster index in the array and add 1 (since cluster indices start at 0). | ||
| const clusterNumber = (data.pointClusters ?? []).reduce<number>((max, clusterIndex) => { | ||
| if (clusterIndex === undefined || clusterIndex < 0) return max | ||
| return Math.max(max, clusterIndex) | ||
| }, 0) + 1 | ||
|
|
||
| this.clustersTextureSize = Math.ceil(Math.sqrt(clusterNumber)) | ||
|
|
||
| if (!this.clusterTexture) this.clusterTexture = reglInstance.texture() | ||
|
|
||
| if (!this.clusterPositionsTexture) this.clusterPositionsTexture = reglInstance.texture() | ||
| if (!this.clusterForceCoefficientTexture) this.clusterForceCoefficientTexture = reglInstance.texture() | ||
| const clusterState = new Float32Array(pointsTextureSize * pointsTextureSize * 4) | ||
| const clusterPositions = new Float32Array(this.clustersTextureSize * this.clustersTextureSize * 4).fill(-1) | ||
| const clusterForceCoefficient = new Float32Array(pointsTextureSize * pointsTextureSize * 4).fill(1) | ||
| if (data.clusterPositions) { | ||
| for (let cluster = 0; cluster < clusterNumber; ++cluster) { | ||
| clusterPositions[cluster * 4 + 0] = data.clusterPositions[cluster * 2 + 0] ?? -1 | ||
| clusterPositions[cluster * 4 + 1] = data.clusterPositions[cluster * 2 + 1] ?? -1 | ||
| } | ||
| } | ||
|
|
||
| for (let i = 0; i < data.pointsNumber; ++i) { | ||
| const clusterIndex = data.pointClusters?.[i] | ||
| if (clusterIndex === undefined) { | ||
| // no cluster, so no forces | ||
| clusterState[i * 4 + 0] = -1 | ||
| clusterState[i * 4 + 1] = -1 | ||
| } else { | ||
| clusterState[i * 4 + 0] = clusterIndex % this.clustersTextureSize | ||
| clusterState[i * 4 + 1] = Math.floor(clusterIndex / this.clustersTextureSize) | ||
| } | ||
|
|
||
| if (data.clusterStrength) clusterForceCoefficient[i * 4 + 0] = data.clusterStrength[i] ?? 1 | ||
| } | ||
| this.clusterTexture({ | ||
| data: clusterState, | ||
| shape: [pointsTextureSize, pointsTextureSize, 4], | ||
| type: 'float', | ||
| }) | ||
| if (!this.clusterFbo) { | ||
| this.clusterFbo = reglInstance.framebuffer({ | ||
| color: this.clusterTexture, | ||
| depth: false, | ||
| stencil: false, | ||
| }) | ||
| } | ||
|
|
||
| this.clusterPositionsTexture({ | ||
| data: clusterPositions, | ||
| shape: [this.clustersTextureSize, this.clustersTextureSize, 4], | ||
| type: 'float', | ||
| }) | ||
| if (!this.clusterPositionsFbo) { | ||
| this.clusterPositionsFbo = reglInstance.framebuffer({ | ||
| color: this.clusterPositionsTexture, | ||
| depth: false, | ||
| stencil: false, | ||
| }) | ||
| } | ||
|
|
||
| this.clusterForceCoefficientTexture({ | ||
| data: clusterForceCoefficient, | ||
| shape: [pointsTextureSize, pointsTextureSize, 4], | ||
| type: 'float', | ||
| }) | ||
| if (!this.clusterForceCoefficientFbo) { | ||
| this.clusterForceCoefficientFbo = reglInstance.framebuffer({ | ||
| color: this.clusterForceCoefficientTexture, | ||
| depth: false, | ||
| stencil: false, | ||
| }) | ||
| } | ||
|
|
||
| if (!this.centermassTexture) this.centermassTexture = reglInstance.texture() | ||
|
|
||
| this.centermassTexture({ | ||
| data: new Float32Array(this.clustersTextureSize * this.clustersTextureSize * 4).fill(0), | ||
| shape: [this.clustersTextureSize, this.clustersTextureSize, 4], | ||
| type: 'float', | ||
| }) | ||
| if (!this.centermassFbo) { | ||
| this.centermassFbo = reglInstance.framebuffer({ | ||
| color: this.centermassTexture, | ||
| depth: false, | ||
| stencil: false, | ||
| }) | ||
| } | ||
|
|
||
| if (!this.pointIndices) this.pointIndices = reglInstance.buffer(0) | ||
| this.pointIndices(createIndexesForBuffer(store.pointsTextureSize)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance validation for cluster indices and positions
Add comprehensive validation to prevent potential issues with invalid cluster indices or positions.
+ private validateClusterData(
+ pointClusters: (number | undefined)[] | undefined,
+ clusterPositions: number[] | undefined,
+ pointsNumber: number | undefined
+ ): boolean {
+ if (!pointClusters && !clusterPositions) return false
+ if (pointsNumber === undefined) return false
+
+ if (pointClusters) {
+ if (pointClusters.length !== pointsNumber) {
+ console.error(`Cluster array length (${pointClusters.length}) doesn't match points number (${pointsNumber})`)
+ return false
+ }
+ }
+
+ if (clusterPositions) {
+ if (clusterPositions.length % 2 !== 0) {
+ console.error('Cluster positions array must have even length')
+ return false
+ }
+ }
+
+ return true
+ }
public create(): void {
const { reglInstance, store, data } = this
const { pointsTextureSize } = store
- if (data.pointsNumber === undefined || (!data.pointClusters && !data.clusterPositions)) return
+ if (!this.validateClusterData(data.pointClusters, data.clusterPositions, data.pointsNumber)) {
+ console.error('Invalid cluster data')
+ return
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public create (): void { | |
| const { reglInstance, store, data } = this | |
| const { pointsTextureSize } = store | |
| if (data.pointsNumber === undefined || (!data.pointClusters && !data.clusterPositions)) return | |
| // Find the highest cluster index in the array and add 1 (since cluster indices start at 0). | |
| const clusterNumber = (data.pointClusters ?? []).reduce<number>((max, clusterIndex) => { | |
| if (clusterIndex === undefined || clusterIndex < 0) return max | |
| return Math.max(max, clusterIndex) | |
| }, 0) + 1 | |
| this.clustersTextureSize = Math.ceil(Math.sqrt(clusterNumber)) | |
| if (!this.clusterTexture) this.clusterTexture = reglInstance.texture() | |
| if (!this.clusterPositionsTexture) this.clusterPositionsTexture = reglInstance.texture() | |
| if (!this.clusterForceCoefficientTexture) this.clusterForceCoefficientTexture = reglInstance.texture() | |
| const clusterState = new Float32Array(pointsTextureSize * pointsTextureSize * 4) | |
| const clusterPositions = new Float32Array(this.clustersTextureSize * this.clustersTextureSize * 4).fill(-1) | |
| const clusterForceCoefficient = new Float32Array(pointsTextureSize * pointsTextureSize * 4).fill(1) | |
| if (data.clusterPositions) { | |
| for (let cluster = 0; cluster < clusterNumber; ++cluster) { | |
| clusterPositions[cluster * 4 + 0] = data.clusterPositions[cluster * 2 + 0] ?? -1 | |
| clusterPositions[cluster * 4 + 1] = data.clusterPositions[cluster * 2 + 1] ?? -1 | |
| } | |
| } | |
| for (let i = 0; i < data.pointsNumber; ++i) { | |
| const clusterIndex = data.pointClusters?.[i] | |
| if (clusterIndex === undefined) { | |
| // no cluster, so no forces | |
| clusterState[i * 4 + 0] = -1 | |
| clusterState[i * 4 + 1] = -1 | |
| } else { | |
| clusterState[i * 4 + 0] = clusterIndex % this.clustersTextureSize | |
| clusterState[i * 4 + 1] = Math.floor(clusterIndex / this.clustersTextureSize) | |
| } | |
| if (data.clusterStrength) clusterForceCoefficient[i * 4 + 0] = data.clusterStrength[i] ?? 1 | |
| } | |
| this.clusterTexture({ | |
| data: clusterState, | |
| shape: [pointsTextureSize, pointsTextureSize, 4], | |
| type: 'float', | |
| }) | |
| if (!this.clusterFbo) { | |
| this.clusterFbo = reglInstance.framebuffer({ | |
| color: this.clusterTexture, | |
| depth: false, | |
| stencil: false, | |
| }) | |
| } | |
| this.clusterPositionsTexture({ | |
| data: clusterPositions, | |
| shape: [this.clustersTextureSize, this.clustersTextureSize, 4], | |
| type: 'float', | |
| }) | |
| if (!this.clusterPositionsFbo) { | |
| this.clusterPositionsFbo = reglInstance.framebuffer({ | |
| color: this.clusterPositionsTexture, | |
| depth: false, | |
| stencil: false, | |
| }) | |
| } | |
| this.clusterForceCoefficientTexture({ | |
| data: clusterForceCoefficient, | |
| shape: [pointsTextureSize, pointsTextureSize, 4], | |
| type: 'float', | |
| }) | |
| if (!this.clusterForceCoefficientFbo) { | |
| this.clusterForceCoefficientFbo = reglInstance.framebuffer({ | |
| color: this.clusterForceCoefficientTexture, | |
| depth: false, | |
| stencil: false, | |
| }) | |
| } | |
| if (!this.centermassTexture) this.centermassTexture = reglInstance.texture() | |
| this.centermassTexture({ | |
| data: new Float32Array(this.clustersTextureSize * this.clustersTextureSize * 4).fill(0), | |
| shape: [this.clustersTextureSize, this.clustersTextureSize, 4], | |
| type: 'float', | |
| }) | |
| if (!this.centermassFbo) { | |
| this.centermassFbo = reglInstance.framebuffer({ | |
| color: this.centermassTexture, | |
| depth: false, | |
| stencil: false, | |
| }) | |
| } | |
| if (!this.pointIndices) this.pointIndices = reglInstance.buffer(0) | |
| this.pointIndices(createIndexesForBuffer(store.pointsTextureSize)) | |
| } | |
| private validateClusterData( | |
| pointClusters: (number | undefined)[] | undefined, | |
| clusterPositions: number[] | undefined, | |
| pointsNumber: number | undefined | |
| ): boolean { | |
| if (!pointClusters && !clusterPositions) return false | |
| if (pointsNumber === undefined) return false | |
| if (pointClusters) { | |
| if (pointClusters.length !== pointsNumber) { | |
| console.error(`Cluster array length (${pointClusters.length}) doesn't match points number (${pointsNumber})`) | |
| return false | |
| } | |
| } | |
| if (clusterPositions) { | |
| if (clusterPositions.length % 2 !== 0) { | |
| console.error('Cluster positions array must have even length') | |
| return false | |
| } | |
| } | |
| return true | |
| } | |
| public create (): void { | |
| const { reglInstance, store, data } = this | |
| const { pointsTextureSize } = store | |
| if (!this.validateClusterData(data.pointClusters, data.clusterPositions, data.pointsNumber)) { | |
| console.error('Invalid cluster data') | |
| return | |
| } | |
| // Find the highest cluster index in the array and add 1 (since cluster indices start at 0). | |
| const clusterNumber = (data.pointClusters ?? []).reduce<number>((max, clusterIndex) => { | |
| if (clusterIndex === undefined || clusterIndex < 0) return max | |
| return Math.max(max, clusterIndex) | |
| }, 0) + 1 | |
| this.clustersTextureSize = Math.ceil(Math.sqrt(clusterNumber)) | |
| if (!this.clusterTexture) this.clusterTexture = reglInstance.texture() | |
| if (!this.clusterPositionsTexture) this.clusterPositionsTexture = reglInstance.texture() | |
| if (!this.clusterForceCoefficientTexture) this.clusterForceCoefficientTexture = reglInstance.texture() | |
| const clusterState = new Float32Array(pointsTextureSize * pointsTextureSize * 4) | |
| const clusterPositions = new Float32Array(this.clustersTextureSize * this.clustersTextureSize * 4).fill(-1) | |
| const clusterForceCoefficient = new Float32Array(pointsTextureSize * pointsTextureSize * 4).fill(1) | |
| if (data.clusterPositions) { | |
| for (let cluster = 0; cluster < clusterNumber; ++cluster) { | |
| clusterPositions[cluster * 4 + 0] = data.clusterPositions[cluster * 2 + 0] ?? -1 | |
| clusterPositions[cluster * 4 + 1] = data.clusterPositions[cluster * 2 + 1] ?? -1 | |
| } | |
| } | |
| for (let i = 0; i < data.pointsNumber; ++i) { | |
| const clusterIndex = data.pointClusters?.[i] | |
| if (clusterIndex === undefined) { | |
| // no cluster, so no forces | |
| clusterState[i * 4 + 0] = -1 | |
| clusterState[i * 4 + 1] = -1 | |
| } else { | |
| clusterState[i * 4 + 0] = clusterIndex % this.clustersTextureSize | |
| clusterState[i * 4 + 1] = Math.floor(clusterIndex / this.clustersTextureSize) | |
| } | |
| if (data.clusterStrength) clusterForceCoefficient[i * 4 + 0] = data.clusterStrength[i] ?? 1 | |
| } | |
| this.clusterTexture({ | |
| data: clusterState, | |
| shape: [pointsTextureSize, pointsTextureSize, 4], | |
| type: 'float', | |
| }) | |
| if (!this.clusterFbo) { | |
| this.clusterFbo = reglInstance.framebuffer({ | |
| color: this.clusterTexture, | |
| depth: false, | |
| stencil: false, | |
| }) | |
| } | |
| this.clusterPositionsTexture({ | |
| data: clusterPositions, | |
| shape: [this.clustersTextureSize, this.clustersTextureSize, 4], | |
| type: 'float', | |
| }) | |
| if (!this.clusterPositionsFbo) { | |
| this.clusterPositionsFbo = reglInstance.framebuffer({ | |
| color: this.clusterPositionsTexture, | |
| depth: false, | |
| stencil: false, | |
| }) | |
| } | |
| this.clusterForceCoefficientTexture({ | |
| data: clusterForceCoefficient, | |
| shape: [pointsTextureSize, pointsTextureSize, 4], | |
| type: 'float', | |
| }) | |
| if (!this.clusterForceCoefficientFbo) { | |
| this.clusterForceCoefficientFbo = reglInstance.framebuffer({ | |
| color: this.clusterForceCoefficientTexture, | |
| depth: false, | |
| stencil: false, | |
| }) | |
| } | |
| if (!this.centermassTexture) this.centermassTexture = reglInstance.texture() | |
| this.centermassTexture({ | |
| data: new Float32Array(this.clustersTextureSize * this.clustersTextureSize * 4).fill(0), | |
| shape: [this.clustersTextureSize, this.clustersTextureSize, 4], | |
| type: 'float', | |
| }) | |
| if (!this.centermassFbo) { | |
| this.centermassFbo = reglInstance.framebuffer({ | |
| color: this.centermassTexture, | |
| depth: false, | |
| stencil: false, | |
| }) | |
| } | |
| if (!this.pointIndices) this.pointIndices = reglInstance.buffer(0) | |
| this.pointIndices(createIndexesForBuffer(store.pointsTextureSize)) | |
| } |
| if (!this.randomValuesTexture) this.randomValuesTexture = reglInstance.texture() | ||
| this.randomValuesTexture({ | ||
| data: randomValuesState, | ||
| shape: [store.pointsTextureSize, store.pointsTextureSize, 4], | ||
| type: 'float', | ||
| }) | ||
| if (!this.randomValuesFbo) { | ||
| this.randomValuesFbo = reglInstance.framebuffer({ | ||
| color: this.randomValuesTexture, | ||
| depth: false, | ||
| stencil: false, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for texture and FBO creation
The texture and FBO creation logic lacks proper error handling. Consider adding error checks and logging.
+ if (!reglInstance) {
+ console.error('reglInstance is not initialized')
+ return
+ }
if (!this.randomValuesTexture) this.randomValuesTexture = reglInstance.texture()
+ if (!this.randomValuesTexture) {
+ console.error('Failed to create randomValuesTexture')
+ return
+ }
this.randomValuesTexture({
data: randomValuesState,
shape: [store.pointsTextureSize, store.pointsTextureSize, 4],
type: 'float',
})
if (!this.randomValuesFbo) {
this.randomValuesFbo = reglInstance.framebuffer({
color: this.randomValuesTexture,
depth: false,
stencil: false,
})
+ if (!this.randomValuesFbo) {
+ console.error('Failed to create randomValuesFbo')
+ return
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!this.randomValuesTexture) this.randomValuesTexture = reglInstance.texture() | |
| this.randomValuesTexture({ | |
| data: randomValuesState, | |
| shape: [store.pointsTextureSize, store.pointsTextureSize, 4], | |
| type: 'float', | |
| }) | |
| if (!this.randomValuesFbo) { | |
| this.randomValuesFbo = reglInstance.framebuffer({ | |
| color: this.randomValuesTexture, | |
| depth: false, | |
| stencil: false, | |
| }) | |
| } | |
| if (!reglInstance) { | |
| console.error('reglInstance is not initialized') | |
| return | |
| } | |
| if (!this.randomValuesTexture) this.randomValuesTexture = reglInstance.texture() | |
| if (!this.randomValuesTexture) { | |
| console.error('Failed to create randomValuesTexture') | |
| return | |
| } | |
| this.randomValuesTexture({ | |
| data: randomValuesState, | |
| shape: [store.pointsTextureSize, store.pointsTextureSize, 4], | |
| type: 'float', | |
| }) | |
| if (!this.randomValuesFbo) { | |
| this.randomValuesFbo = reglInstance.framebuffer({ | |
| color: this.randomValuesTexture, | |
| depth: false, | |
| stencil: false, | |
| }) | |
| if (!this.randomValuesFbo) { | |
| console.error('Failed to create randomValuesFbo') | |
| return | |
| } | |
| } |
| import findHoveredPointVert from '@/graph/modules/Points/find-hovered-point.vert' | ||
| import fillGridWithSampledNodesFrag from '@/graph/modules/Points/fill-sampled-nodes.frag' | ||
| import fillGridWithSampledNodesVert from '@/graph/modules/Points/fill-sampled-nodes.vert' | ||
| import { createSizeBufferAndFillSizeStore } from '@/graph/modules/Points/size-buffer' | ||
| import fillGridWithSampledPointsFrag from '@/graph/modules/Points/fill-sampled-points.frag' | ||
| import fillGridWithSampledPointsVert from '@/graph/modules/Points/fill-sampled-points.vert' | ||
| import updatePositionFrag from '@/graph/modules/Points/update-position.frag' | ||
| import { createIndexesBuffer, createQuadBuffer, destroyFramebuffer } from '@/graph/modules/Shared/buffer' | ||
| import { createTrackedIndicesBuffer, createTrackedPositionsBuffer } from '@/graph/modules/Points/tracked-buffer' | ||
| import { createIndexesForBuffer, createQuadBuffer, destroyFramebuffer } from '@/graph/modules/Shared/buffer' | ||
| import trackPositionsFrag from '@/graph/modules/Points/track-positions.frag' | ||
| import dragPointFrag from '@/graph/modules/Points/drag-point.frag' | ||
| import updateVert from '@/graph/modules/Shared/quad.vert' | ||
| import clearFrag from '@/graph/modules/Shared/clear.frag' | ||
| import { readPixels } from '@/graph/helper' | ||
| import { CosmosInputNode, CosmosInputLink } from '@/graph/types' | ||
|
|
||
| export class Points<N extends CosmosInputNode, L extends CosmosInputLink> extends CoreModule<N, L> { | ||
| export class Points extends CoreModule { | ||
| public currentPositionFbo: regl.Framebuffer2D | undefined |
There was a problem hiding this comment.
Revise discount and fee structure
The current implementation has a critical business logic flaw:
- Adding a flat $20 fee after applying the discount can result in a higher final bill than the original amount for small purchases
- For example: $100 purchase with 10% discount = $90 + $20 fee = $110 final bill
This negates the benefit of the loyalty discount for small purchases.
Consider these alternatives:
- Apply the fee before the discount
- Scale the fee based on purchase amount
- Set a minimum purchase amount for the fee
- Remove the fee for loyal customers (3+ years)
Would you like me to propose a revised implementation?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
package.json (2)
13-18: Clarify build system strategyThe project now maintains two build systems (Rollup and Vite). This might lead to confusion about which one to use. Consider:
- Documenting the purpose of each build system
- Explaining when to use
buildvsbuild:vite- If possible, standardize on a single build system to reduce maintenance overhead
95-98: Consider enhancing ESLint configurationWhile the Storybook ESLint plugin is correctly configured, consider enhancing the configuration with:
- Project-specific rules
- TypeScript-specific rules from
@typescript-eslint- Import ordering rules to maintain consistent code style
"eslintConfig": { "extends": [ + "plugin:@typescript-eslint/recommended", "plugin:storybook/recommended" - ] + ], + "rules": { + "import/order": ["error", { + "groups": ["builtin", "external", "internal", "parent", "sibling", "index"], + "newlines-between": "always" + }] + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
package.json(3 hunks)
🔇 Additional comments (2)
package.json (2)
3-3: Major changes in licensing and versioning
- Version bump to 2.0.0-beta.20 indicates breaking changes
- License change from CC-BY-NC-4.0 to MIT is significant:
- MIT is more permissive
- Allows commercial use
- May affect existing users
Please ensure:
- All contributors have agreed to the license change
- Breaking changes are documented in CHANGELOG.md
- Migration guide is provided for users
Also applies to: 8-8
72-72: Update TypeScript version and verify lock file inclusion
The TypeScript version (5.5.2) specified in package.json is a beta version, while the current stable version is 5.7.2. Consider updating to the stable version.
| "@storybook/addon-essentials": "^8.4.5", | ||
| "@storybook/addon-interactions": "^8.4.5", | ||
| "@storybook/blocks": "^8.4.5", | ||
| "@storybook/html": "^8.4.5", | ||
| "@storybook/html-vite": "^8.4.5", | ||
| "@storybook/test": "^8.4.5", |
There was a problem hiding this comment.
💡 Codebase verification
Storybook integration is properly configured
The verification shows a complete Storybook setup:
.storybookdirectory is present with all essential configuration files (main.ts, preview.ts, manager.ts)- Story files exist for key components in
src/stories/with proper structure and metadata - Stories are organized into meaningful categories (Examples/Clusters, Examples/Experiments)
However, Storybook documentation is missing from the README. Consider adding:
- Instructions for running Storybook locally
- Information about available story categories
- Guidelines for adding new stories
🔗 Analysis chain
Verify Storybook integration completeness
The Storybook integration looks comprehensive with all necessary dependencies properly aligned at version 8.4.5. However, please ensure:
- Storybook configuration files are included in the PR
- Example stories are provided for key components
- Documentation about running and building Storybook is added to README
Also applies to: 71-71
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for Storybook configuration files
echo "Checking for Storybook configuration:"
fd -e js -e ts ".storybook"
# Check for story files
echo -e "\nChecking for story files:"
fd -e stories.tsx -e stories.ts
Length of output: 280
Script:
#!/bin/bash
# Check for .storybook directory and its contents
echo "Checking .storybook directory contents:"
fd ".storybook"
# Check README for Storybook documentation
echo -e "\nChecking README for Storybook mentions:"
rg -i "storybook" README.md
# Look at the content of story files to verify they are properly structured
echo -e "\nChecking content of story files:"
cat src/stories/clusters.stories.ts
echo -e "\n-------------------\n"
cat src/stories/experiments.stories.ts
Length of output: 3112
Script:
#!/bin/bash
# Check for .storybook configuration directory at root level
echo "Checking for .storybook directory at root:"
ls -la .storybook 2>/dev/null || echo "No .storybook directory found"
# Check for any storybook config files in other locations
echo -e "\nChecking for any storybook config files:"
fd -e js -e ts -e cjs -e mjs "main|preview|manager"
# Check for additional story files that might be nested
echo -e "\nChecking for additional story files:"
rg -l "\.stories\." --type ts --type tsx
Length of output: 832
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores