feat: Create visualizer for generator steps using cytoscape #412
Conversation
ENG-529 Create visualizer for generator tasks / dependencies
Allows us to better refactor the generator by understanding the dependency structure |
🦋 Changeset detectedLatest commit: 1d30997 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe pull request introduces several enhancements across multiple packages, focusing on improving generator functionality, metadata handling, and visualization. Key changes include adding graph visualization for generator steps using Cytoscape.js, refactoring the Changes
Sequence DiagramsequenceDiagram
participant Generator
participant DependencySort
participant GeneratorRunner
participant HTMLWriter
Generator->>DependencySort: Get sorted run steps
DependencySort-->>Generator: Return steps with metadata
Generator->>GeneratorRunner: Execute generator entry
GeneratorRunner->>GeneratorRunner: Build output with metadata
alt BASEPLATE_WRITE_GENERATOR_STEPS_HTML is true
GeneratorRunner->>HTMLWriter: Write generator steps HTML
HTMLWriter-->>GeneratorRunner: HTML file generated
end
The sequence diagram illustrates the new workflow for generator step processing, including metadata generation and optional HTML visualization. 🪧 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 (
|
kingston
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
packages/project-builder-server/src/sync/generator-steps-html-writer.ts (2)
6-75: Consider allowing external script versions to be configurable or pinned.
Embedding Cytoscape from a CDN is convenient, but pinning a more specific version or utilizing Subresource Integrity (SRI) can help maintain reliability and security.
86-112: Add error handling or logging for file writes.
WhilewriteGeneratorStepsHtmlis concise, you could consider wrappingfs.writeFilein a try/catch block to make errors more explicit. Also, verifyingoutputDirectorybefore writing might be beneficial in complex scenarios.packages/sync/src/core/engine/generator-runner.ts (1)
120-129: Optional labeling of nodes and edges may aid debugging.
Currently, each node'slabelis not populated. If this is intentional, it’s fine; otherwise, consider adding a descriptive label property for improved clarity in visualizations.packages/sync/src/core/engine/dependency-sort.ts (2)
169-174: Consider removing duplicates infullSteps.You’re aggregating steps from different sources (
entriesplusinterdependentNodes). If there’s any overlap, callingtoposort.array()might produce errors. Ensure duplicates won't exist or deduplicate before callingtoposort.
175-181: Potential fallback for empty graphs.When
fullStepsorfullEdgesis empty,toposortmight return an empty list. Ensure your consumer code can handle that scenario (e.g., no steps to run). Otherwise, a fallback might be necessary.packages/project-builder-server/src/sync/index.ts (2)
83-83: Path reorganization check.Moving the build result file to
baseplate/build/last_build_result.jsonis a logical step for clarity. Ensure that any documentation or references to the old path are updated accordingly.
198-198: Suggest a debug log here.A short debug or info log could confirm that generation of the steps HTML was skipped, helping users understand why the file wasn’t written if they expected it.
packages/sync/src/utils/create-generator-with-tasks.ts (1)
56-65: Optional return fromruncan lead to confusion.When
ExportMapis empty, this method can returnvoid. This might confuse future maintainers if they expect aSimpleGeneratorTaskInstance. Consider adding documentation or examples clarifying the scenarios in which a task returns nothing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (13)
.changeset/rotten-rocks-report.md(1 hunks).changeset/small-ghosts-smash.md(1 hunks)packages/core-generators/src/generators/node/node-git-ignore/index.ts(1 hunks)packages/fastify-generators/src/generators/auth0/auth0-module/index.ts(0 hunks)packages/fastify-generators/src/generators/yoga/yoga-plugin/index.ts(0 hunks)packages/project-builder-server/src/service/environment-flags.ts(1 hunks)packages/project-builder-server/src/sync/generator-steps-html-writer.ts(1 hunks)packages/project-builder-server/src/sync/index.ts(4 hunks)packages/react-generators/src/generators/core/react/index.ts(1 hunks)packages/sync/src/core/engine/dependency-sort.ts(2 hunks)packages/sync/src/core/engine/generator-runner.ts(2 hunks)packages/sync/src/core/generator-output.ts(1 hunks)packages/sync/src/utils/create-generator-with-tasks.ts(4 hunks)
💤 Files with no reviewable changes (2)
- packages/fastify-generators/src/generators/yoga/yoga-plugin/index.ts
- packages/fastify-generators/src/generators/auth0/auth0-module/index.ts
🔇 Additional comments (16)
packages/project-builder-server/src/service/environment-flags.ts (1)
1-6: Consider clarifying the intended behavior of the boolean toggle.
Currently,!!process.env.BASEPLATE_WRITE_GENERATOR_STEPS_HTMLwill treat any non-empty environment variable value astrue. If you intend for this flag to betrueonly when explicitly set to"true", consider an explicit check instead, e.g.:process.env.BASEPLATE_WRITE_GENERATOR_STEPS_HTML === 'true'packages/project-builder-server/src/sync/generator-steps-html-writer.ts (1)
77-85: Consider more descriptive naming or error handling incleanLabel.
If the regex fails to match or the string is unexpected, the function returns the originalid. Confirm this fallback is the desired behavior, or if additional logging/validation would be helpful.packages/sync/src/core/engine/generator-runner.ts (1)
29-32: Excellent approach to collecting metadata directly with the steps.
DestructuringmetadatafromgetSortedRunStepskeeps the logic clean. Just ensure it’s always returned even in potential edge cases with minimal or empty tasks.packages/sync/src/core/generator-output.ts (2)
136-139: Well-structured metadata model for generator steps.
ThegeneratorStepNodesandgeneratorStepEdgesfields neatly capture graph data. This will be easy to extend in the future if you decide to track additional properties.
145-145: Good use of an optional metadata property.
Opting in or out ofmetadatahelps maintain backward compatibility while providing richer data when available.packages/sync/src/core/engine/dependency-sort.ts (1)
137-140: Ensure consistency with the documented return type.The updated function signature returns both
stepsandmetadata. Ensure that all callers ofgetSortedRunStepscorrectly handle the newly introducedmetadataproperty, especially in code that previously expected just a string array.packages/react-generators/src/generators/core/react/index.ts (1)
110-110: Revisiting file-watch scope.You broadened the ignore pattern to all of
baseplate/**. Verify that this won’t omit necessary updates or critical ephemeral files for local development, especially if certain files inbaseplatemight still need to be watched.packages/project-builder-server/src/sync/index.ts (4)
11-11: Good use of environment flags.Using
environmentFlagsprevents cluttering the main codebase with environment-specific logic. The approach is cleaner and more testable.
14-14: Validate import usage ofwriteGeneratorStepsHtml.This import is only used when
BASEPLATE_WRITE_GENERATOR_STEPS_HTMListrue. Confirm that tree shaking or bundling tools won’t remove it before runtime. You may also want to guard its usage more explicitly if the import might cause overhead.
100-103: Confirm newly introduced path for clean temporary files.You relocated
.clean_tmptobuild/clean_tmp. Confirm if any code or scripts outside this file rely on the old location. This might cause unexpected clean-up issues.
192-197: Great extension of optional HTML output.Conditionally writing the generator steps HTML is a helpful addition. This ensures minimal overhead for most users, while enabling a richer visualization for those who opt in.
packages/sync/src/utils/create-generator-with-tasks.ts (2)
44-46: Refined type defaults are beneficial.Allowing
ExportMap,DependencyMap, andTaskDependencyMapto default to an empty record streamlines the usage of the interface, especially when tasks don’t produce or consume exports. This is a clean improvement.
136-138: Broadening defaults at the task builder level.Setting default types here greatly reduces boilerplate in user code. Just ensure there’s no silent type mismatch if tasks unexpectedly return providers.
.changeset/small-ghosts-smash.md (1)
1-6: LGTM! Appropriate versioning for the changes.The patch version bump is correctly applied for a backwards-compatible enhancement that allows optional returns from the run function.
.changeset/rotten-rocks-report.md (1)
1-6: LGTM! Versioning aligns with feature scope.The patch version bump is appropriate for adding new visualization capabilities without breaking changes. The description clearly communicates the addition of Cytoscape.js for generator steps visualization, which directly addresses the PR objectives.
packages/core-generators/src/generators/node/node-git-ignore/index.ts (1)
53-54: LGTM! Improved organization of build artifacts.The consolidation of Baseplate artifacts under a single
baseplate/builddirectory improves maintainability.Let's verify that this change won't affect existing projects:
✅ Verification successful
Verified: Safe to consolidate build artifacts under baseplate/build
The only reference to old patterns is in
vite.config.tsfor Vite's watch settings, which serves a different purpose than git ignore patterns. No existing.gitignorefiles will be affected by this change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for existing .gitignore files that might be affected by this change # Search for .gitignore files containing the old patterns echo "Checking for .gitignore files with old patterns..." rg -l "baseplate/\.clean_tmp|baseplate/\.build_result\.json" --type gitignore # Check if any repositories are using the old paths echo "Checking for actual usage of old paths..." rg -l "baseplate/\.clean_tmp|baseplate/\.build_result\.json" --type tsLength of output: 431
Script:
#!/bin/bash # Search .gitignore files correctly echo "Checking .gitignore files..." fd -t f .gitignore$ | xargs rg "baseplate/\.clean_tmp|baseplate/\.build_result\.json" # Check the content of vite.config.ts echo -e "\nChecking vite.config.ts content..." cat "tests/simple/packages/web/vite.config.ts"Length of output: 1295
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- task.run may return undefined | ||
| const runResult = task.run(dependencies, resolvedTaskOutputs) ?? {}; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Confirm fallback logic.
The fallback of runResult ?? {} might obscure issues if the run method is intended to return a complex structure. If tasks mistakenly return undefined in an unexpected scenario, debugging can be tricky. Adding a debug log or assertion could be helpful.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/sync/src/core/engine/dependency-sort.unit.test.ts (2)
34-34: Ensure comprehensive test coverage formetadata.
While verifyingstepsis sufficient for checking the order, consider adding assertions to confirm that the returnedmetadataproperty (if any) also has the desired structure.
129-129: Check for partial completion vs. final steps.
You may consider verifying partial steps or intermediate states if applicable, ensuring that all expected steps are included.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (1)
packages/sync/src/core/engine/dependency-sort.unit.test.ts(7 hunks)
🔇 Additional comments (5)
packages/sync/src/core/engine/dependency-sort.unit.test.ts (5)
10-10: Good job accessing the newstepsproperty.
No issues with the updated assertion for an empty list.
43-43: Fine approach for testing reversed dependencies.
No further suggestion as the coverage for reversed dependency order is clearly tested.
87-87: Excellent: new structure verification is consistent.
All good here; the new assertion follows the updated return structure.
202-202: Validation of complex inter-dependencies is sound.
Appropriate assertion for the final step order, no concerns.
254-254: Looks great for second-level inter-dependencies.
No change needed. The test is thorough for multiple layers of tasks.
kingston
deleted the
kingston/eng-529-create-visualizer-for-generator-tasks-dependencies
branch
Summary by CodeRabbit
Release Notes
New Features
runfunction.Improvements
Changes
.gitignoreexclusions for Baseplate build artifacts.