Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Storybook: Destroy Cosmos when switching stories #129

Merged
merged 1 commit into from
Dec 27, 2024
Merged

Storybook: Destroy Cosmos when switching stories #129

merged 1 commit into from
Dec 27, 2024

Conversation

Stukova
Copy link
Contributor

@Stukova Stukova commented Dec 27, 2024

Refactored the story components to properly destroy the Cosmos instance when switching between stories. This change ensures that each story cleans up its own Cosmos instance, preventing potential memory leaks.

Summary by CodeRabbit

  • New Features

    • Enhanced story configuration for clusters and experiments in Storybook.
    • Introduced a new createStory function for improved story management.
  • Bug Fixes

    • Updated return types for story functions to include both graph and div elements.
  • Documentation

    • Improved clarity of story exports and configurations.

@Stukova Stukova requested a review from rokotyan December 27, 2024 10:04
Copy link

coderabbitai bot commented Dec 27, 2024

Walkthrough

The pull request introduces a comprehensive refactoring of story generation in the project. The changes focus on standardizing the return type of story functions to include both the graph and div elements, creating a new createStory utility, and updating import statements across multiple files. This modification enhances the consistency of story creation and provides more comprehensive data management for graph-based stories in the Storybook environment.

Changes

File Change Summary
src/stories/clusters.stories.ts Updated story imports and exports using new createStory function
src/stories/clusters/radial.ts Modified RadialStory to return { graph, div } instead of just div
src/stories/clusters/with-labels.ts Updated return type to { graph, div } and added Graph import
src/stories/clusters/worm.ts Changed WormStory to return { graph, div }
src/stories/create-cosmos.ts Enhanced CosmosStoryProps type with additional optional properties
src/stories/create-story.ts Added new Story type and createStory utility function
src/stories/experiments.stories.ts Refactored story exports using createStory
src/stories/experiments/full-mesh.ts Updated FullMeshStory to return { graph, div }
src/stories/experiments/mesh-with-holes.ts Modified MeshWithHolesStory to return { graph, div }

Sequence Diagram

sequenceDiagram
    participant Story as Story Function
    participant CreateCosmos as createCosmos()
    participant Graph as Graph Instance
    participant Storybook as Storybook Renderer

    Story Function->>CreateCosmos: Call with configuration
    CreateCosmos-->>Story Function: Return { graph, div }
    Story Function->>Storybook Renderer: Return { graph, div }
    Storybook Renderer->>Graph: Render and manage lifecycle
Loading

Poem

🐰 A Rabbit's Tale of Graph Stories Bright

With cosmos dancing, data takes flight
Stories now return both graph and view
Storybook magic, clean and true
Refactored code, a programmer's delight!

🌟 Hop, hop, hooray! 🚀

Tip

CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command @coderabbitai generate docstrings to have CodeRabbit automatically generate docstrings for your pull request. We would love to hear your feedback on Discord.


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b087d54 and 082c1f7.

📒 Files selected for processing (9)
  • src/stories/clusters.stories.ts (2 hunks)
  • src/stories/clusters/radial.ts (1 hunks)
  • src/stories/clusters/with-labels.ts (2 hunks)
  • src/stories/clusters/worm.ts (1 hunks)
  • src/stories/create-cosmos.ts (0 hunks)
  • src/stories/create-story.ts (1 hunks)
  • src/stories/experiments.stories.ts (2 hunks)
  • src/stories/experiments/full-mesh.ts (1 hunks)
  • src/stories/experiments/mesh-with-holes.ts (1 hunks)
💤 Files with no reviewable changes (1)
  • src/stories/create-cosmos.ts
🚧 Files skipped from review as they are similar to previous changes (7)
  • src/stories/experiments/mesh-with-holes.ts
  • src/stories/clusters/radial.ts
  • src/stories/clusters/with-labels.ts
  • src/stories/clusters.stories.ts
  • src/stories/experiments.stories.ts
  • src/stories/create-story.ts
  • src/stories/experiments/full-mesh.ts
🔇 Additional comments (3)
src/stories/clusters/worm.ts (3)

2-2: LGTM! The updated signature enables proper cleanup.

The changes to expose the Graph instance in the return type will allow proper cleanup when switching between stories, preventing memory leaks.

Also applies to: 6-6


Line range hint 6-18: Verify story cleanup implementation.

While exposing the Graph instance enables cleanup, we should verify that the cleanup is properly implemented in the Storybook configuration.

Let's check for cleanup implementation in story configuration files:

✅ Verification successful

Story cleanup is properly implemented

The cleanup is correctly implemented in src/stories/create-story.ts which handles the cleanup of all stories through a common implementation. The code shows that the Graph instance is properly destroyed when stories are unmounted:

{
  cleanup: () => {
    d.args.graph?.destroy()
  }
}

The Graph's destroy() method in src/index.ts performs comprehensive cleanup:

  • Clears timeouts
  • Stops animation frames
  • Destroys the WebGL context
  • Cleans up the canvas
  • Removes FPS monitor and related styles
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Look for story cleanup implementation
# Search for story cleanup patterns in configuration and story files

# Check for story cleanup patterns
rg -A 10 "cleanup|destroy|dispose|unmount" --type ts

# Look for story configuration files
fd "\.stories\.(ts|tsx|js|jsx)$" --exec cat {}

Length of output: 10173


Line range hint 9-18: Verify simulation parameters for the worm visualization.

The implementation looks correct, but please verify that the simulation parameters (gravity: 0.5, repulsion: 1, linkSpring: 1) are optimal for the worm-like visualization pattern.

Let's check if these parameters are consistent across similar visualizations:

✅ Verification successful

Simulation parameters for worm visualization are intentionally different from defaults

The worm visualization's parameters differ from the default values in create-cosmos.ts for a specific visual effect:

  • Default values: gravity: 0.02, repulsion: 0.5, linkSpring: 2
  • Worm values: gravity: 0.5, repulsion: 1, linkSpring: 1

These parameters are consistent with other cluster visualizations, as seen in with-labels.ts which uses similar values (gravity: 0.5, linkSpring: 1). The increased gravity and repulsion in the worm visualization help create the desired worm-like pattern by:

  • Higher gravity (0.5) pulls points more strongly toward the center
  • Balanced repulsion (1.0) and linkSpring (1.0) maintain the worm's shape while preventing collapse
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check simulation parameters in other story files
# Look for createCosmos calls with simulation parameters

rg -A 5 "simulationGravity|simulationRepulsion|simulationLinkSpring" --type ts

Length of output: 8387


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.

Actionable comments posted: 4

🧹 Nitpick comments (68)
src/modules/ForceMouse/index.ts (1)

7-7: Add JSDoc or inline comments for the runCommand property.

Although this property is straightforward, consider adding a brief comment describing its purpose (e.g., storing the draw command for the force calculation). This will improve maintainability and clarity for future contributors.

src/modules/ForceManyBodyQuadtree/index.ts (1)

27-48: Efficient handling of multiple levels
The logic to create a texture and FBO for each level is clear. Ensure that the level key generation (level[${i}]) remains consistent throughout usage, and that any missing cleanup does not lead to leaked GPU resources.

src/modules/Lines/index.ts (2)

20-59: Conditionally initialized draw command
The logic checks if drawCurveCommand is absent before creating it. This helps avoid double-initialization. For future reference, consider re-initializing if user config changes or if you need multiple draw modes.


151-153: Arrow buffer logic
Same fallback approach for arrow data. This helps maintain consistent attribute definitions.

src/modules/ForceLink/index.ts (1)

66-120: Creation of textures and draw command
Creating separate textures for link indices and their properties (bias, strength, distance) is neatly organized. Consider lazy-loading or partial updates if the dataset is very large and only changes partially.

src/modules/Clusters/index.ts (2)

105-118: Reusing or creating framebuffers
Consistent with other modules, you create or reuse FBO references. That avoids reinitialization overhead but be sure to handle scenario changes (e.g., cluster count changes mid-session) that might require a bigger texture.


194-199: Overall run pipeline
Running clearCentermassCommand, calculateCentermassCommand, then applyForcesCommand is consistent with typical GPU-based solve steps. Consider adding an early return or fallback if partial data is updated to avoid leftover states.

src/modules/ForceManyBody/index.ts (2)

20-21: Check for resource cleanup
These new fields (randomValuesTexture and pointIndices) do not appear to have an explicit destroy/cleanup process. If they hold GPU resources, consider freeing them on module teardown to avoid potential memory leaks.


114-147: Iterative force command
Defining the force logic for each quadtree level is neatly separated. You might consider merging calculations for performance, but the current approach is clear and maintainable.

src/modules/GraphData/index.ts (3)

48-51: pointsNumber getter
Returning this.pointPositions.length / 2 is reasonable. Consider returning 0 instead of undefined for a more homogeneous return type.


52-54: linksNumber getter
Likewise, consider returning 0 if links are not defined, for consistency.


186-195: updateLinkStrength
Sets this.linkStrength to undefined if input lengths don’t match. Confirm if a default fallback is desirable here, similar to widths and colors.

src/config.ts (2)

Line range hint 16-456: Point-centric configuration
Renaming config from node-centric to point-centric is a major shift. The new properties (pointColor, pointSize, etc.) align well with the rest of the codebase. Keep in mind that advanced customization beyond uniform sizes (e.g., custom scaling logic) must remain documented.


Line range hint 535-552: Deep merge logic
Merging config objects allows partial updates. The approach is straightforward. Keep an eye on potential performance overhead for large nested config objects.

src/modules/Points/index.ts (4)

2-3: Commented-out imports
These D3 imports are commented out. If they are no longer used, consider removing them to reduce dead code. Otherwise, keep them commented for future reference.


49-50: Size, indices textures
Again, be consistent in calling destroy() if needed. If the Points module is re-created, watch for stale GPU objects.


514-517: drag
Running the drag command twice is interesting—likely ensures pixel-perfect alignment. Just confirm no performance penalty.


609-638: Commented-out rescaleInitialNodePositions
This code remains commented. If it’s no longer needed, consider removing or rewriting it for the new approach.

src/index.ts (1)

Line range hint 51-64: Hovered point detection
Refactors hovered point logic to store _findHoveredPointExecutionCount. This helps throttle GPU picking calls.

src/modules/Points/fill-sampled-points.frag (1)

5-5: Verify usage of varying qualifier in newer GLSL versions.

While varying is valid in GLSL ES 1.0, note that in more recent GLSL versions, the in qualifier replaces varying. If you plan to upgrade to newer GLSL ES (e.g., 3.0) or desktop GLSL, consider updating the qualifier accordingly.

src/modules/ForceCenter/calculate-centermass.vert (1)

13-15: Consider making point size configurable.

Hardcoding 1.0 for gl_PointSize might not cover all use cases. You could expose it as a uniform variable to allow dynamic sizing in different contexts.

src/modules/Points/drag-point.frag (1)

5-7: Consider improving uniform naming clarity.

Using a generic name like index could cause confusion. Renaming it to something like draggedIndex or activeIndex may enhance readability.

src/stories/create-story.ts (1)

7-10: Promote stronger type safety
The storyFunction is returning both a graph and div, which is correct. It might be beneficial to define a type or interface for this return object, so that storyFunction: () => { graph: Graph; div: HTMLDivElement } could be replaced with a custom type for improved maintainability in the future.

src/stories/clusters/radial.ts (1)

13-24: Maintain cohesion with the createCosmos properties
The arguments passed into createCosmos are comprehensive for a radial layout. Consider whether any specialized radial configuration or layout code can be extracted into a reusable helper if you plan to create more radial-based stories in the future.

src/modules/ForceCenter/force-center.frag (1)

14-16: Potential runtime checks for uniform values.

The shader calculates forces based on alpha, centerForce, and centermassValues. Consider clamping or validating these inputs if out-of-range values might lead to unwanted or unstable behaviors.

Also applies to: 22-22

src/modules/Points/draw-points.frag (2)

11-11: Minor spelling consistency.
Smoothing control the smoothness” should read “Smoothing controls the smoothness” for clarity.

-// Smoothing control the smoothness of the point’s edge
+// Smoothing controls the smoothness of the point’s edge

18-18: Consider clarifying the coordinate commentary.
A small addition here to mention that gl_PointCoord ranges from [0, 1] might help future readers.

-// Calculate coordinates within the point
+// Calculate coordinates within the point (gl_PointCoord ranges from 0.0 to 1.0)
src/stories/clusters/with-labels.ts (1)

7-8: Exporting the story properly.
Exposing this function for Storybook seems correct. Consider whether a more descriptive function name (e.g., clusterLabelsStory) would benefit future readers.

src/stories/experiments.stories.ts (1)

42-43: ESLint rule disabled for default export.
Ensure that this aligns with your team’s style guidelines. If your codebase generally prefers named exports, consider synergy across all story files.

src/modules/Clusters/force-cluster.frag (2)

1-3: Ensure consistent precision specification.

You're correctly specifying highp float under #ifdef GL_ES. If you plan to support other GL environments (e.g., WebGL 2 or desktop GL), consider adding conditional checks or fallback precision specifiers to guarantee consistency across different platforms.


17-39: Consider incorporating alpha or meta information into your output.

Currently, gl_FragColor = velocity; implies RGBA = (velocity.x, velocity.y, velocity.z, velocity.w). If you need to use alpha or distinct color channels, ensure that it aligns with your rendering pipeline. For improved clarity, you might store velocity in the RGB channels while reserving the alpha channel for additional data (e.g., debugging).

src/modules/ForceManyBody/force-centermass.frag (3)

1-2: Enhance in-file documentation.

The opening comments provide a quick overview of the shader’s functionality. Consider including more detail about how random factors and point positions contribute to the final velocity output.


Line range hint 18-38: Promote code reusability for center-of-mass calculations.

calculateAdditionalVelocity includes distance and angle calculations that appear in multiple force shaders. If you plan to maintain multiple force-based fragment shaders, consider extracting these computations into a shared module or library of shader functions for consistency and easier maintenance.


41-50: Guard against very large random influences.

Lines 42–48 factor random values directly into velocity. If random values can be large, you may inadvertently create extreme velocities. Consider clamping or scaling random values to avoid unusual or unpredictable movement.

src/modules/Points/draw-points.vert (2)

18-21: Limit usage of varyings.

textureCoords, rgbColor, and alpha are sensible varyings, but ensure you only pass what’s strictly needed to the fragment shader. Unused varyings can increase memory bandwidth between vertex and fragment stages.


25-35: Simplify scaling logic.

calculatePointSize includes separate scaling paths for zooming and minimal/maximum constraints. You might unify these steps with a single expression to reduce branching, e.g., applying a clamp or mix function. This can improve shader readability and performance.

src/stories/clusters.stories.ts (2)

1-6: Organize imports for clarity.

You’re importing several story modules (WithLabelsStory, WormStory, RadialStory) and types (Meta, CosmosStoryProps). Consider grouping external dependencies, then local ones, to provide a clear structure for future maintainers.


58-59: Lint exception note.

// eslint-disable-next-line import/no-default-export is sometimes necessary for Storybook integrations. Confirm that your lint rules are consistently applied to avoid confusion for collaborators unfamiliar with these exceptions.

src/helper.ts (1)

49-52: Check parameter type for clarity

Declaring value as number | undefined | null | typeof NaN might be confusing since NaN is already a number. Consider consolidating to number | undefined | null.

-export function isNumber (value: number | undefined | null | typeof NaN): boolean {
+export function isNumber (value: number | undefined | null): boolean {
   return value !== undefined && value !== null && !Number.isNaN(value)
 }
src/modules/Points/find-hovered-point.vert (1)

44-44: Fine-tune default radius scaling

Using 0.5 * calculatePointSize(...) is a suitable approach. If needed, consider making the 0.5 factor a uniform or constant for additional flexibility.

src/stories/create-cluster-labels.ts (1)

9-37: Dynamically creating label elements

Creating and styling divs and p tags for each cluster is well-structured. The usage of modern fonts is appropriate, but watch out for fallback chains to ensure consistent styling across browsers.

src/modules/Points/draw-highlighted.vert (1)

47-50: Relative ring radius logic

Applying a * relativeRingRadius to the base size is fine for highlighted points. A separate uniform for ring thickness might make this more configurable.

src/stories/create-cosmos.ts (1)

18-66: Ensure consistent cleanup or disposal.

createCosmos sets up and configures a Graph instance, which is great for dynamic usage in stories. However, for memory-safety and story-to-story transitions, consider exposing or returning a cleanup/destroy method in addition to the div and graph objects. This ensures that any GPU-accelerated resources or event listeners are properly released.

src/modules/ForceLink/force-spring.ts (2)

26-32: Parameterize the loop limit for performance optimization.

You're using a loop for (float i = 0.0; i < MAX_LINKS; i += 1.0). This can cause a performance overhead on some GLSL platforms. If feasible, consider chunking or breaking up link operations. Alternatively, confirm that hardware can efficiently handle loops up to maxLinks.


51-56: Consider factoring out repeated logic.

From lines 51–56, you apply the same principle to compute distance, clamp it with randomMinLinkDist, and scale it. Reusing a function or inlining a well-documented snippet can reduce duplication across multiple force calculations if they follow a similar pattern.

src/modules/ForceManyBodyQuadtree/quadtree-frag-shader.ts (1)

41-41: Check for repeated logic blocks.

If the same logic snippet is repeated in multiple condition branches, it may be beneficial to encapsulate it in a dedicated function or small inline macro, so repeated usage can be quickly identified and revised.

src/modules/ForceManyBody/force-level.frag (5)

Line range hint 23-37: Efficient distance-based velocity calculation.

  1. The function name calculateAdditionalVelocity accurately conveys its purpose.
  2. The normalization approach to compute repulsion is clear and mathematically sound.
  3. Ensure that values like distanceMin2 are well-documented for future maintainability.

60-60: Clarity in loop boundary condition.

The comment regarding iterating over levels helps future readers understand the hierarchical approach used. Keep such descriptive comments consistent for multi-level loops.


94-94: Repeated pattern for velocity calculations.

The same pattern appears in multiple nested loops. Consider extracting repeated logic into a function or macro if possible for improved maintainability.


108-108: Maintain modular approach for velocity updates.

Repeated calls to the same function are cohesive, but be mindful of GPU cost if expansions are planned. Potential separation of concerns (accumulate velocity vs. apply velocity) could improve performance.


115-115: Suggest a buffer approach for performance.

Repeated texture fetch and function calls might benefit from a consolidated approach (e.g., iterative accumulation into a buffer). Evaluate if it’s worth the added complexity.

src/modules/Lines/draw-curve-line.vert (1)

55-55: Good descriptive comment for the link distance calculation.

Maintaining stable documentation around geometry logic is invaluable. Nice to see the clarifying comment.

src/stories/generate-mesh-data.ts (4)

5-7: Small utility for random ranges.

getRandom is useful and straightforward. Consider a more robust approach (e.g., seeded random) if deterministic sequences are needed for reproducible results.


15-28: Avoid large inline interfaces in loops.

The MeshData type is well-structured. If the data structure grows, consider splitting into separate interfaces for clarity.


50-56: Reasonable default space size.

Using 8192 as spaceSize is fairly large. Just confirm there are no GPU constraints for massive point sets.


58-90: Cluster position & link creation logic.

  1. Placing cluster centers on a circle introduces a natural radial distribution.
  2. The probability-based link creation is straightforward but can produce highly random structures—document for story consumers.
src/modules/Store/index.ts (3)

13-13: Transition from a node-based model to a point-based model is seamless.
The removal of generic parameters <N> aligns with the new approach. Keep an eye on references to old node-based logic in dependent files, if any.


74-77: Scales domain/range
Documenting the logic behind dividing the adjusted space size might be beneficial for maintainability. Otherwise, no issues found.


90-94: setHoveredPointRingColor
Updating only the first three array elements is sensible. If alpha channel changes become necessary later, ensure you update the entire array.

.github/SECURITY.md (1)

1-13: Minor wording adjustment for formal tone
In line 9, consider replacing "fix the issue" with "address the issue" for more official language.

- This gives us time to work with you to fix the issue before public exposure...
+ This gives us time to work with you to address the issue before public exposure...
🧰 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)

.github/workflows/github_pages.yml (1)

38-40: Consider using LTS version of Node.js

While Node.js 20.x is the current version, consider using an LTS version for better stability in CI/CD pipelines. Also, the explicit npm version installation might not be necessary as Node.js 20.x comes with a recent npm version.

-          node-version: '20.x'
+          node-version: '18.x'
-      - run: npm install -g npm@10
package.json (1)

13-13: Consider adding test script for Storybook

While build scripts for Storybook are added, consider adding a test script to ensure stories work correctly, especially for cleanup scenarios.

  "scripts": {
    "build:vite": "rm -rf dist; vite build",
    "storybook": "storybook dev -p 6006",
    "build:storybook": "storybook build",
+   "test:storybook": "test-storybook --url http://localhost:6006"
  },

Also applies to: 71-75

cosmos-2-0-migration-notes.md (1)

77-91: Enhance section headers formatting

The sections marked with emphasis (**) should be proper markdown headers for better documentation structure.

-**Summary of Additional Changes**
+### Summary of Additional Changes

-**Before**
+#### Before

-**After**
+#### After

Also applies to: 93-98

🧰 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)

README.md (1)

110-112: Add Storybook usage documentation

Since Storybook support is being added, consider documenting it in the documentation section.

 - 🛠 [Configuration](https://github.com/cosmograph-org/cosmos/wiki/Cosmos-v2-(Beta-version)-configuration)
 - ⚙️ [API Reference](https://github.com/cosmograph-org/cosmos/wiki/Cosmos-v2-(Beta-version)-API-Reference)
 - 🚀 [Migration Guide](https://github.com/cosmograph-org/cosmos/tree/next/cosmos-2-0-migration-notes.md)
+- 📚 [Storybook Guide](https://github.com/cosmograph-org/cosmos/wiki/Using-Cosmos-in-Storybook)
src/stories/2. configuration.mdx (2)

10-10: Improve clarity of the disableSimulation property description.

The description should be clearer about when and how this property affects the Cosmos instance, especially in the context of story switching.

Apply this diff to enhance the description:

-| 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 | Controls whether the simulation runs or if the graph is rendered statically. When enabled, Cosmos uses the provided `x` and `y` values to position points, or assigns random positions if coordinates are not specified. IMPORTANT: This property must be set during initialization as it cannot be changed later via `setConfig`. This is particularly relevant when managing multiple story instances to prevent simulation conflicts. | `false` |
🧰 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)


35-35: Add missing useQuadtree compatibility information.

The documentation should clearly state the compatibility requirements for the useQuadtree feature.

Apply this diff to add compatibility information:

-| useQuadtree | Use the classic [quadtree algorithm](https://en.wikipedia.org/wiki/Barnes%E2%80%93Hut_simulation) for the Many-Body force. This property will be applied only on component initialization and it can't be changed using the `setConfig` method.<br /><br /> ⚠ The algorithm might not work on some GPUs (e.g. Nvidia) and on Windows (unless you disable ANGLE in the browser settings). | `false` |
+| useQuadtree | Use the classic [quadtree algorithm](https://en.wikipedia.org/wiki/Barnes%E2%80%93Hut_simulation) for the Many-Body force. This property will be applied only on component initialization and it can't be changed using the `setConfig` method.<br /><br /> ⚠ **Compatibility Warning**: The algorithm has known issues with:<br/>1. Specific GPUs (e.g., Nvidia)<br/>2. Windows systems with ANGLE enabled<br/>3. Certain WebGL implementations<br/><br/>Before enabling this feature, ensure compatibility with your target environment and consider providing fallback options. | `false` |
🧰 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)

CODE_OF_CONDUCT.md (1)

3-4: Fix grammatical error in the opening sentence.

There's a subject-verb agreement error in the opening sentence.

Apply this diff to fix the grammar:

-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
+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 its
src/stories/3. api-reference.mdx (1)

296-296: Fix typo in spaceToScreenPosition method description.

The word "method" is repeated.

Apply this diff to fix the typo:

-This method method is used to convert X and Y point coordinates from the space coordinate system (which is typically used for graph data) to the screen coordinate system (which is used for rendering the graph on the display).
+This method is used to convert X and Y point coordinates from the space coordinate system (which is typically used for graph data) to the screen coordinate system (which is used for rendering the graph on the display).
🧰 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)

🛑 Comments failed to post (4)
src/stories/1. welcome.mdx (1)

38-59: 🛠️ Refactor suggestion

Add cleanup instructions for proper Cosmos instance management

Given that this PR focuses on proper destruction of Cosmos instances when switching stories, it would be valuable to add a section about cleanup best practices. This helps prevent memory leaks and ensures proper resource management.

Add a cleanup section after the current example:

 graph.render()
+
+// Cleanup section
+### Proper Cleanup
+
+When you're done with the graph or need to recreate it:
+```javascript
+// Destroy the graph instance to free up resources
+graph.destroy()
+```
+
+> **Important**: Always destroy the graph instance when it's no longer needed,
+> especially in frameworks like React or when using Storybook to prevent memory leaks.
cosmos-2-0-migration-notes.md (1)

34-49: 🛠️ Refactor suggestion

Add cleanup instructions for Storybook integration

Since this PR focuses on Cosmos cleanup in Storybook, consider adding a section about proper cleanup in Storybook stories.

Add the following section:

#### Cleanup in Storybook

When using Cosmos in Storybook, ensure proper cleanup in your stories:

```js
export const MyStory = () => {
  const div = document.createElement('div');
  const graph = new Graph(div, config);
  
  // Cleanup function
  return () => {
    graph.destroy(); // Ensure this is called when switching stories
  };
};

<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 LanguageTool</summary>

[uncategorized] ~36-~36: Possible missing comma found.
Context: ...uration Updates  Accessor functions for styling such as `nodeColor`, `nodeSize`, `linkC...

(AI_HYDRA_LEO_MISSING_COMMA)

</details>

</details>

</blockquote></details>
<details>
<summary>README.md (1)</summary><blockquote>

84-84: _:warning: Potential issue_

**Fix broken migration guide link**

The migration guide link appears to be incorrect.

```diff
-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.
📝 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.

Check the [Migration Guide](https://github.com/cosmograph-org/cosmos/blob/main/cosmos-2-0-migration-notes.md) for details.
src/stories/3. api-reference.mdx (1)

373-375: 🛠️ Refactor suggestion

Enhance documentation for the destroy() method.

Given this PR's focus on proper Cosmos instance destruction during story switches, the destroy() method documentation should be more comprehensive.

Apply this diff to improve the documentation:

-### <a name="destroy" href="#destroy">#</a> graph.<b>destroy</b>()
-
-Destroys the current Cosmos instance.
+### <a name="destroy" href="#destroy">#</a> graph.<b>destroy</b>()
+
+Destroys the current Cosmos instance, cleaning up all resources and event listeners. This method should be called when:
+
+* Unmounting components that use Cosmos
+* Switching between stories in Storybook
+* Before creating a new instance with `create()`
+
+**Important**: Failing to call `destroy()` before unmounting or switching stories can lead to:
+* Memory leaks
+* Event listener conflicts
+* WebGL context issues
+* Unexpected behavior in subsequent instances
+
+**Example:**
+```javascript
+// Clean up when unmounting
+useEffect(() => {
+  return () => {
+    graph.destroy();
+  };
+}, [graph]);
+```

@rokotyan rokotyan merged commit 1656162 into main Dec 27, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants