feat: Improve dependency traversal for interface inheritance #14
Conversation
This commit introduces several improvements to the dependency traversal logic, specifically handling interface inheritance scenarios: 1. Correctly extracts dependencies from interfaces that inherit from other interfaces, ensuring the correct order of dependencies. 2. Handles multiple interface inheritance, where an interface extends multiple other interfaces. 3. Supports nested interface inheritance, where an interface extends an interface that extends another interface. These changes ensure that the dependency traversal algorithm can accurately identify and order the dependencies for complex interface inheritance structures, which is crucial for generating correct code.
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds interactive graph visualization and visualization API, expands dependency extraction to aliases/interfaces/enums/functions (including interface inheritance), refactors generateCode signature, makes NodeGraph edge addition idempotent, removes unused-node pruning, updates docs, adds Graphology/ForceAtlas2 deps, and introduces traversal tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant API as index.generateVisualization
participant TS as SourceFileBuilder
participant DT as DependencyTraversal
participant GV as GraphVisualizer
User->>API: generateVisualization(options)
API->>TS: build SourceFile from options
TS-->>API: sourceFile
API->>DT: startTraversal(sourceFile)
DT-->>API: traversed node graph
API->>DT: visualizeGraph(visualizationOptions)
DT->>GV: generateVisualization(nodeGraph, options)
GV-->>API: outputPath (HTML)
API-->>User: Promise<string> (HTML path)
sequenceDiagram
autonumber
participant DT as DependencyTraversal
participant AST as TS AST Nodes
participant NG as NodeGraph
DT->>AST: visit node
alt TypeAlias
DT->>AST: analyze TypeNode for references
else Interface
DT->>AST: collect properties + extends clauses
else Enum / Function
DT->>AST: collect referenced types
end
DT->>NG: ensure node exists
loop for each referenced type
DT->>NG: addDependency(ref -> current) [guarded, idempotent]
end
DT-->>DT: compute ordering (topological or insertion-order fallback)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/traverse/dependency-traversal.ts (1)
271-288: Support typeof references in types (TypeQuery).Dependencies via typeof (e.g., typeof createUser) aren’t captured. Add TypeQuery handling.
if (Node.isTypeReference(node)) { const typeRefNode = node as TypeReferenceNode const typeName = typeRefNode.getTypeName().getText() ... } + // Handle `typeof Foo` used in type positions + if (Node.isTypeQueryNode && Node.isTypeQueryNode(node)) { + const baseName = node.getExprName().getText().split('.').pop()! + for (const qualifiedName of this.nodeGraph.nodes()) { + const nodeData = this.nodeGraph.getNode(qualifiedName) + if (nodeData.originalName === baseName) { + references.push(qualifiedName) + break + } + } + }
🧹 Nitpick comments (13)
ARCHITECTURE.md (2)
73-78: Minor grammar fixes in “Dependency Extraction” bullets.Tighten phrasing and articles.
-3. **Dependency Extraction**: Analyzes type references to build dependency graph for all supported TypeScript constructs: +3. **Dependency Extraction**: Analyzes type references to build a dependency graph for all supported TypeScript constructs: - - **Type Aliases**: Extracts dependencies from type alias declarations + - **Type Aliases**: Extracts dependencies from type-alias declarations - - **Interfaces**: Analyzes interface property types and heritage clauses + - **Interfaces**: Analyzes interface property types and heritage clauses - - **Enums**: Processes enum member value dependencies + - **Enums**: Processes enum-member value dependencies - - **Functions**: Extracts dependencies from parameter types, return types, and type parameters + - **Functions**: Extracts dependencies from parameter types, return types, and type parameters
80-101: Document CDN vs. package usage and expected environment.The section implies loading Sigma programs via CDN. Clarify:
- Whether local packages are required or the HTML exclusively uses CDN modules.
- That visualization is browser-only (WebGL) and not needed for core codegen, to avoid confusion for headless/CI environments.
Add a short “Runtime assumptions” note.
tests/traverse/simple-dependency.test.ts (1)
5-39: Strengthen assertions to ensure exact ordering and membership.Add an equality check to prevent accidental extras/omissions.
- // Check correct ordering + // Check correct ordering and exact membership expect(userIdIndex).toBeLessThan(userIndex) expect(userIndex).toBeLessThan(usersIndex) expect(userIdIndex).toBeLessThan(usersIndex) + expect(result.map((n) => n.originalName)).toEqual(['UserId', 'User', 'Users'])src/traverse/dependency-traversal.ts (1)
275-286: Name matching is O(N) per reference and ambiguous across modules.Scanning all nodes and matching by originalName is slow and can mis-link when multiple modules share a name. Build an index or resolve by symbol.
I can propose a minimal Map<string, string[]> index built once in extractDependencies. Would you like a patch?
Also applies to: 301-307
tests/traverse/dependency-ordering.test.ts (2)
95-96: Avoid shadowing the shared Project instance.Redundant const project = new Project() hides the beforeEach project and can lead to inconsistent setup.
- const project = new Project() + // use the project from beforeEach- const project = new Project() + // use the project from beforeEachAlso applies to: 119-120
150-153: Assert the full chain ordering for clarity.Also check Entity < Entities to fully cover the intended ordering.
expect(entityIdIndex).toBeLessThan(entitiesIndex) + expect(entityIndex).toBeLessThan(entitiesIndex)tests/traverse/circular-dependency.test.ts (1)
32-34: Add assertion for base interface in generic extends.Item extends EntityInfo should also order EntityInfo before Item; this guards the generic-extends path.
Add:
const entityInfoIndex = result.findIndex((n) => n.originalName === 'EntityInfo') const itemIndex = result.findIndex((n) => n.originalName === 'Item') expect(entityInfoIndex).toBeGreaterThanOrEqual(0) expect(entityInfoIndex).toBeLessThan(itemIndex)This will fail until extends resolution handles generics (see src fix).
Also applies to: 48-63
tests/traverse/interface-inheritance-dependency.test.ts (3)
36-39: Use the return value of startTraversal directly.No need to call getNodesToPrint() again.
- traverser.startTraversal(sourceFile) - const dependencies = traverser.getNodesToPrint() + const dependencies = traverser.startTraversal(sourceFile)- traverser.startTraversal(sourceFile) - const dependencies = traverser.getNodesToPrint() + const dependencies = traverser.startTraversal(sourceFile)- traverser.startTraversal(sourceFile) - const dependencies = traverser.getNodesToPrint() + const dependencies = traverser.startTraversal(sourceFile)Also applies to: 71-73, 100-102
7-10: Deduplicate helper across tests.getNodeName appears in multiple suites; consider moving to tests/utils to avoid repetition.
61-88: Add a generics case to multiple inheritance.A small extension (e.g., interface C extends A, B) would ensure generic params don’t affect ordering.
I can add a compact test if you want.
tests/traverse/dependency-extraction.test.ts (1)
7-10: Deduplicate helper across tests.Move getNodeName to tests/utils to remove duplication.
src/index.ts (1)
54-57: Remove or use the unusedvisualizeflag inCodeGenerationOptions.
visualize?: booleanisn't read anywhere. Either drop it to avoid API noise or makegenerateCodeoptionally trigger visualization.export interface CodeGenerationOptions extends InputOptions { - visualize?: boolean visualizationOptions?: VisualizationOptions }src/utils/graph-visualizer.ts (1)
229-231: Consider pinning CDN integrity and versions.
Add SRI and exact versions to improve supply-chain safety and reproducibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
ARCHITECTURE.md(1 hunks)package.json(1 hunks)src/index.ts(2 hunks)src/traverse/dependency-traversal.ts(4 hunks)src/traverse/node-graph.ts(1 hunks)src/utils/graph-visualizer.ts(1 hunks)tests/traverse/circular-dependency.test.ts(1 hunks)tests/traverse/dependency-extraction.test.ts(1 hunks)tests/traverse/dependency-ordering.test.ts(2 hunks)tests/traverse/dependency-traversal.integration.test.ts(1 hunks)tests/traverse/interface-inheritance-dependency.test.ts(1 hunks)tests/traverse/simple-dependency.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
tests/traverse/circular-dependency.test.ts (1)
src/traverse/dependency-traversal.ts (1)
DependencyTraversal(26-319)
tests/traverse/simple-dependency.test.ts (1)
src/traverse/dependency-traversal.ts (1)
DependencyTraversal(26-319)
tests/traverse/interface-inheritance-dependency.test.ts (3)
src/traverse/types.ts (1)
TraversedNode(5-12)src/traverse/dependency-traversal.ts (1)
DependencyTraversal(26-319)tests/utils.ts (1)
createSourceFile(12-14)
tests/traverse/dependency-ordering.test.ts (1)
src/traverse/dependency-traversal.ts (1)
DependencyTraversal(26-319)
src/traverse/dependency-traversal.ts (3)
src/traverse/types.ts (1)
TraversedNode(5-12)src/utils/graph-visualizer.ts (2)
VisualizationOptions(6-9)GraphVisualizer(32-424)src/traverse/node-graph.ts (1)
NodeGraph(8-39)
src/utils/graph-visualizer.ts (1)
src/traverse/types.ts (1)
TraversedNode(5-12)
tests/traverse/dependency-extraction.test.ts (3)
src/traverse/types.ts (1)
TraversedNode(5-12)src/traverse/dependency-traversal.ts (1)
DependencyTraversal(26-319)tests/utils.ts (1)
createSourceFile(12-14)
src/index.ts (3)
src/input-handler.ts (2)
InputOptions(5-10)createSourceFileFromInput(64-101)src/utils/graph-visualizer.ts (2)
VisualizationOptions(6-9)generateVisualization(36-54)src/traverse/dependency-traversal.ts (1)
DependencyTraversal(26-319)
🪛 LanguageTool
ARCHITECTURE.md
[grammar] ~73-~73: There might be a mistake here.
Context: ...traction**: Analyzes type references to build dependency graph for all supported Type...
(QB_NEW_EN)
[grammar] ~73-~73: There might be a mistake here.
Context: ...for all supported TypeScript constructs: - Type Aliases: Extracts dependencies fr...
(QB_NEW_EN)
🔇 Additional comments (8)
tests/traverse/dependency-traversal.integration.test.ts (1)
11-11: Rename aligns terminology across suite.The updated suite name improves clarity and doesn’t affect behavior. LGTM.
src/traverse/node-graph.ts (1)
29-38: Idempotent edge addition is a solid improvement.Guarding missing nodes and duplicate edges will stabilize traversal builds. Nice.
src/traverse/dependency-traversal.ts (1)
38-41: Confirm intent: direct imports marked as isMainCode=true.collectFromImports(..., true) labels directly imported declarations as main code; only transitive imports become non-main. If visualize/printing relies on isMainCode to prioritize main-file declarations, this may skew results.
Do you want direct imports to be considered main code? If not, pass false here.
tests/traverse/dependency-ordering.test.ts (1)
41-92: LGTM: ordering expectations align with traversal intent.These cases validate simple and multi-usage chains well.
src/index.ts (3)
8-8: LGTM on type-only import.
No issues with importing VisualizationOptions for API typing.
59-69: OK to reuse traversal; ensure package export is wired.
generateVisualizationcorrectly reusesDependencyTraversal. Verify it’s exported in package exports/types so consumers can import it.
71-74: LGTM on simplified parameter handling.
Switching from destructuring to passing theInputOptionsobject directly is non-breaking and clearer.src/utils/graph-visualizer.ts (1)
229-231: Use the correct UMD globals for Graphology and Sigma
• Graphology v0.26.0 exports a single globalgraphologyobject (notGraph), withDirectedGraphas a top-level property. Replace any use ofwindow.graphology.Graphornew Graph.DirectedGraph()with:const { DirectedGraph } = window.graphology; const graph = new DirectedGraph();• Sigma v3.0.2 exposes only
window.Sigma(no lowercasesigma). Use:const Sigma = window.Sigma; // add “|| window.sigma” only if you must support older bundles
package.json
Outdated
| "@sigma/node-border": "^3.0.0", | ||
| "@sigma/node-square": "^3.0.0", | ||
| "graphology": "^0.26.0", | ||
| "graphology-dag": "^0.4.1", | ||
| "graphology-layout-forceatlas2": "^0.10.1", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Validate need for new runtime deps; consider dev/optional or CDN-only.
If the visualization HTML loads Sigma node programs via unpkg at runtime (as ARCHITECTURE.md suggests), keeping @sigma/node-border, @sigma/node-square, and graphology-layout-forceatlas2 as hard runtime dependencies may be unnecessary and increases install footprint for consumers who won't use visualization.
- If they’re not imported in Node code, remove them or move to
devDependencies. - If only used to type-check/bundle examples, prefer devDeps; otherwise document why they must be runtime deps.
Run to confirm actual imports:
Optionally move to devDeps:
"dependencies": {
- "@sigma/node-border": "^3.0.0",
- "@sigma/node-square": "^3.0.0",
"graphology": "^0.26.0",
"graphology-dag": "^0.4.1",
- "graphology-layout-forceatlas2": "^0.10.1",
"graphology-traversal": "^0.3.1"
},
+ "devDependencies": {
+ "@sigma/node-border": "^3.0.0",
+ "@sigma/node-square": "^3.0.0",
+ "graphology-layout-forceatlas2": "^0.10.1",
+ ...
+ }🏁 Script executed:
#!/bin/bash
rg -n "@sigma/node-(border|square)|graphology-layout-forceatlas2" -g '!**/node_modules/**' -C2Length of output: 3581
Move @sigma/node-border and @sigma/node-square to devDependencies
They’re not imported anywhere in the codebase and—as ARCHITECTURE.md describes—are loaded via unpkg at runtime; retain graphology-layout-forceatlas2 in dependencies because it’s imported in src/utils/graph-visualizer.ts.
🤖 Prompt for AI Agents
In package.json around lines 38 to 42, two packages (@sigma/node-border and
@sigma/node-square) are listed in dependencies but are not imported and are
loaded at runtime via unpkg per ARCHITECTURE.md; move "@sigma/node-border" and
"@sigma/node-square" from "dependencies" into "devDependencies" and leave
"graphology-layout-forceatlas2" (and other graphology packages) in
"dependencies" since graphology-layout-forceatlas2 is imported in
src/utils/graph-visualizer.ts; after updating package.json, run your package
manager (npm install or yarn) to update the lockfile.
| static async generateVisualization( | ||
| nodeGraph: DirectedGraph<TraversedNode>, | ||
| options: VisualizationOptions = {}, | ||
| ): Promise<string> { | ||
| const { outputPath = './dependency-graph.html', title = 'TypeScript Dependency Graph' } = | ||
| options | ||
|
|
||
| const { nodes, edges } = this.convertGraphToSigmaData(nodeGraph) | ||
|
|
||
| // Always apply ForceAtlas2 layout | ||
| this.applyForceAtlas2Layout(nodes) | ||
|
|
||
| const htmlContent = this.generateHTMLContent(nodes, edges, title) | ||
|
|
||
| const file = Bun.file(outputPath) | ||
| await file.write(htmlContent) | ||
|
|
||
| return outputPath | ||
| } |
There was a problem hiding this comment.
Do not depend on Bun runtime; use Node’s fs/promises.
Bun.file(...).write(...) breaks in Node environments. Use fs/promises and ensure the output directory exists.
+import { promises as fs } from 'node:fs'
+import { dirname } from 'node:path'
@@
- const htmlContent = this.generateHTMLContent(nodes, edges, title)
-
- const file = Bun.file(outputPath)
- await file.write(htmlContent)
+ const htmlContent = this.generateHTMLContent(nodes, edges, title)
+ await fs.mkdir(dirname(outputPath), { recursive: true })
+ await fs.writeFile(outputPath, htmlContent, 'utf8')📝 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.
| static async generateVisualization( | |
| nodeGraph: DirectedGraph<TraversedNode>, | |
| options: VisualizationOptions = {}, | |
| ): Promise<string> { | |
| const { outputPath = './dependency-graph.html', title = 'TypeScript Dependency Graph' } = | |
| options | |
| const { nodes, edges } = this.convertGraphToSigmaData(nodeGraph) | |
| // Always apply ForceAtlas2 layout | |
| this.applyForceAtlas2Layout(nodes) | |
| const htmlContent = this.generateHTMLContent(nodes, edges, title) | |
| const file = Bun.file(outputPath) | |
| await file.write(htmlContent) | |
| return outputPath | |
| } | |
| // At the top of src/utils/graph-visualizer.ts | |
| import { promises as fs } from 'node:fs' | |
| import { dirname } from 'node:path' | |
| export class GraphVisualizer { | |
| static async generateVisualization( | |
| nodeGraph: DirectedGraph<TraversedNode>, | |
| options: VisualizationOptions = {}, | |
| ): Promise<string> { | |
| const { outputPath = './dependency-graph.html', title = 'TypeScript Dependency Graph' } = | |
| options | |
| const { nodes, edges } = this.convertGraphToSigmaData(nodeGraph) | |
| // Always apply ForceAtlas2 layout | |
| this.applyForceAtlas2Layout(nodes) | |
| - const htmlContent = this.generateHTMLContent(nodes, edges, title) | |
| - | |
| - const file = Bun.file(outputPath) | |
| const htmlContent = this.generateHTMLContent(nodes, edges, title) | |
| // Ensure the output directory exists | |
| await fs.mkdir(dirname(outputPath), { recursive: true }) | |
| // Write the HTML to the specified file | |
| await fs.writeFile(outputPath, htmlContent, 'utf8') | |
| return outputPath | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/utils/graph-visualizer.ts around lines 36 to 54, the code uses
Bun.file(...).write(...) which is Bun-specific and will fail in Node; replace
this with Node's fs/promises: import writeFile and mkdir from 'fs/promises' and
path from 'path', compute the output directory with path.dirname(outputPath),
call mkdir(dir, { recursive: true }) to ensure it exists, then await
writeFile(outputPath, htmlContent, 'utf8'); finally return outputPath.
src/utils/graph-visualizer.ts
Outdated
| private static calculateImportNestingLevel(nodeData: TraversedNode): number { | ||
| if (nodeData.isMainCode) { | ||
| return 4 // Highest intensity for main code | ||
| } | ||
|
|
||
| if (nodeData.isImported) { | ||
| // Simple heuristic: count path depth | ||
| const filePath = nodeData.node.getSourceFile().getFilePath() | ||
| const pathDepth = filePath.split('/').length | ||
| return Math.max(1, Math.min(3, pathDepth - 3)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix path splitting for Windows compatibility.
filePath.split('/') fails on Windows paths. Normalize and split by platform separator.
+import path from 'node:path'
@@
- const filePath = nodeData.node.getSourceFile().getFilePath()
- const pathDepth = filePath.split('/').length
+ const filePath = nodeData.node.getSourceFile().getFilePath()
+ const pathDepth = path.normalize(filePath).split(path.sep).length
return Math.max(1, Math.min(3, pathDepth - 3))📝 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 static calculateImportNestingLevel(nodeData: TraversedNode): number { | |
| if (nodeData.isMainCode) { | |
| return 4 // Highest intensity for main code | |
| } | |
| if (nodeData.isImported) { | |
| // Simple heuristic: count path depth | |
| const filePath = nodeData.node.getSourceFile().getFilePath() | |
| const pathDepth = filePath.split('/').length | |
| return Math.max(1, Math.min(3, pathDepth - 3)) | |
| // At the top of src/utils/graph-visualizer.ts, alongside your other imports | |
| import path from 'node:path' | |
| … | |
| private static calculateImportNestingLevel(nodeData: TraversedNode): number { | |
| if (nodeData.isMainCode) { | |
| return 4 // Highest intensity for main code | |
| } | |
| if (nodeData.isImported) { | |
| // Simple heuristic: count path depth | |
| const filePath = nodeData.node.getSourceFile().getFilePath() | |
| const pathDepth = path.normalize(filePath).split(path.sep).length | |
| return Math.max(1, Math.min(3, pathDepth - 3)) | |
| } | |
| // …rest of method… | |
| } |
🤖 Prompt for AI Agents
In src/utils/graph-visualizer.ts around lines 202 to 211, the code uses
filePath.split('/') which breaks on Windows; change to normalize the path and
split by the platform separator (import path from 'path'), e.g. const parts =
path.normalize(filePath).split(path.sep).filter(Boolean) and use parts.length
for pathDepth so the depth calculation works cross-platform.
| ): string { | ||
| return `<!DOCTYPE html> | ||
| <html> | ||
| <head> | ||
| <title>${title}</title> | ||
| <script src="https://cdn.jsdelivr.net/npm/graphology@0.26.0/dist/graphology.umd.min.js"></script> | ||
| <script src="https://cdn.jsdelivr.net/npm/sigma@3.0.2/dist/sigma.min.js"></script> | ||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Escape title to avoid HTML injection.
Users can pass arbitrary title. Escape it before templating.
- ): string {
- return `<!DOCTYPE html>
+ ): string {
+ const safeTitle = String(title)
+ .replace(/&/g, '&')
+ .replace(/</g, '<')
+ .replace(/>/g, '>')
+ .replace(/"/g, '"')
+ .replace(/'/g, ''')
+ return `<!DOCTYPE html>
<html>
<head>
- <title>${title}</title>
+ <title>${safeTitle}</title>📝 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.
| ): string { | |
| return `<!DOCTYPE html> | |
| <html> | |
| <head> | |
| <title>${title}</title> | |
| <script src="https://cdn.jsdelivr.net/npm/graphology@0.26.0/dist/graphology.umd.min.js"></script> | |
| <script src="https://cdn.jsdelivr.net/npm/sigma@3.0.2/dist/sigma.min.js"></script> | |
| ): string { | |
| const safeTitle = String(title) | |
| .replace(/&/g, '&') | |
| .replace(/</g, '<') | |
| .replace(/>/g, '>') | |
| .replace(/"/g, '"') | |
| .replace(/'/g, ''') | |
| return `<!DOCTYPE html> | |
| <html> | |
| <head> | |
| <title>${safeTitle}</title> | |
| <script src="https://cdn.jsdelivr.net/npm/graphology@0.26.0/dist/graphology.umd.min.js"></script> | |
| <script src="https://cdn.jsdelivr.net/npm/sigma@3.0.2/dist/sigma.min.js"></script> |
🤖 Prompt for AI Agents
In src/utils/graph-visualizer.ts around lines 224 to 231, the template injects
the raw title into the HTML which allows HTML injection; create or reuse a small
escapeHtml function that replaces &, <, >, ", and ' (and optionally /) with
their corresponding HTML entities and call it on title before interpolation
(e.g. const safeTitle = escapeHtml(title)); then use safeTitle in the template
string so the generated HTML cannot be exploited by injected markup or scripts.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/traverse/dependency-traversal.ts (2)
37-41: Don’t mark imported nodes as main code.
collectFromImports(importDeclarations, true)flags first-level imports asisMainCode, skewing visualization and sizing. Passfalse.- const importDeclarations = mainSourceFile.getImportDeclarations() - this.collectFromImports(importDeclarations, true) + const importDeclarations = mainSourceFile.getImportDeclarations() + this.collectFromImports(importDeclarations, false)
268-279: Normalize TypeReference names (handles namespaces).
getTypeName().getText()can bens.Base; compare againstoriginalNameby stripping qualification.- if (Node.isTypeReference(node)) { - const typeName = node.getTypeName().getText() + if (Node.isTypeReference(node)) { + const typeName = node.getTypeName().getText().split('.').pop()!src/index.ts (1)
70-90: Update call sites and docs forgenerateCode(options)signature
- tests/utils.ts (around line 30): replace
withconst code = generateCode(source, filePath)const code = generateCode({ sourceCode: source, callerFile: filePath })- ARCHITECTURE.md (examples at lines 256–258 and 265–267): wrap the former positional args in an options object.
Migration note:
In vX.Y.Z we changed
generateCodefromgenerateCode(sourceCode: string, filePath?: string)to
generateCode(options: { sourceCode: string; callerFile?: string; filePath?: string; })Replace all calls accordingly and update any code samples in docs to use the new
optionsshape.
♻️ Duplicate comments (4)
src/traverse/dependency-traversal.ts (1)
297-315: Fix interface extends resolution for generics/namespaces.Using
typeNode.getText()yieldsBase<T>orns.Base, failing to matchoriginalName. Extract base identifier from the expression.- for (const typeNode of heritageClause.getTypeNodes()) { - const typeName = typeNode.getText() + for (const typeNode of heritageClause.getTypeNodes()) { + const typeName = typeNode.getExpression().getText().split('.').pop()!src/utils/graph-visualizer.ts (3)
47-50: Good: FA2 layout now considers edges.Using connectivity improves positioning quality.
45-57: Remove Bun dependency and duplicate write; use fs/promises only.
Bun.file(...).write(...)breaks in Node and you also write twice. Switch to async fs and create the directory before writing.-import { mkdirSync, writeFileSync } from 'node:fs' -import { dirname } from 'node:path' +import { promises as fs } from 'node:fs' +import { dirname } from 'node:path' @@ - const htmlContent = this.generateHTMLContent(nodes, edges, title) - - const file = Bun.file(outputPath) - await file.write(htmlContent) - - mkdirSync(dirname(outputPath), { recursive: true }) - writeFileSync(outputPath, htmlContent, 'utf8') + const htmlContent = this.generateHTMLContent(nodes, edges, title) + await fs.mkdir(dirname(outputPath), { recursive: true }) + await fs.writeFile(outputPath, htmlContent, 'utf8')
232-237: Escape HTML title to prevent injection.User-provided
titleis injected raw.- ): string { - return `<!DOCTYPE html> + ): string { + const safeTitle = String(title) + .replace(/&/g, '&') + .replace(/</g, '<') + .replace(/>/g, '>') + .replace(/"/g, '"') + .replace(/'/g, ''') + return `<!DOCTYPE html> @@ - <title>${title}</title> + <title>${safeTitle}</title>
🧹 Nitpick comments (3)
tests/traverse/interface-inheritance-dependency.test.ts (1)
88-114: Add tests for generics and qualified names in extends.Current traversal uses string text for extends; a regression could drop deps like
C extends A<string>orC extends ns.A. Add tests to lock this down.Example tests to append:
test('should handle generic base in extends', () => { const sourceFile = createSourceFile(project, ` interface A<T> { a: T } interface C extends A<string> { c: boolean } `) const deps = traverser.startTraversal(sourceFile).map(getNodeName) expect(deps).toEqual(expect.arrayContaining(['A', 'C'])) expect(deps.indexOf('A')).toBeLessThan(deps.indexOf('C')) }) test('should handle qualified name in extends', () => { const sourceFile = createSourceFile(project, ` namespace ns { export interface A { a: string } } interface C extends ns.A { c: boolean } `) const deps = traverser.startTraversal(sourceFile).map(getNodeName) expect(deps).toEqual(expect.arrayContaining(['A', 'C'])) expect(deps.indexOf('A')).toBeLessThan(deps.indexOf('C')) })src/traverse/dependency-traversal.ts (2)
260-324: De-duplicate collected references to reduce redundant edge checks.Prevent repeated pushes when the same type is encountered multiple times.
- private extractTypeReferences(node: Node): string[] { - const references: string[] = [] + private extractTypeReferences(node: Node): string[] { + const references = new Set<string>() @@ - references.push(qualifiedName) + references.add(qualifiedName) @@ - references.push(qualifiedName) + references.add(qualifiedName) @@ - references.push(qualifiedName) + references.add(qualifiedName) @@ - return references + return Array.from(references)
239-244: Minor: prefer isAcyclic for clarity.Functionally equivalent to
!hasCycle, but reads clearer.-import { hasCycle, topologicalSort } from 'graphology-dag' +import { isAcyclic, topologicalSort } from 'graphology-dag' @@ - const nodes = hasCycle(this.nodeGraph) + const nodes = isAcyclic(this.nodeGraph) - ? Array.from(this.nodeGraph.nodes()) - : topologicalSort(this.nodeGraph) + ? topologicalSort(this.nodeGraph) + : Array.from(this.nodeGraph.nodes())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
package.json(1 hunks)src/index.ts(2 hunks)src/traverse/dependency-traversal.ts(5 hunks)src/traverse/node-graph.ts(1 hunks)src/utils/graph-visualizer.ts(1 hunks)tests/traverse/dependency-extraction.test.ts(1 hunks)tests/traverse/dependency-ordering.test.ts(2 hunks)tests/traverse/interface-inheritance-dependency.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/traverse/dependency-extraction.test.ts
- package.json
- src/traverse/node-graph.ts
- tests/traverse/dependency-ordering.test.ts
🧰 Additional context used
🧬 Code graph analysis (4)
src/traverse/dependency-traversal.ts (3)
src/traverse/types.ts (1)
TraversedNode(5-12)src/utils/graph-visualizer.ts (2)
VisualizationOptions(8-11)GraphVisualizer(34-432)src/traverse/node-graph.ts (1)
NodeGraph(8-40)
tests/traverse/interface-inheritance-dependency.test.ts (3)
src/traverse/types.ts (1)
TraversedNode(5-12)src/traverse/dependency-traversal.ts (1)
DependencyTraversal(25-325)tests/utils.ts (1)
createSourceFile(12-14)
src/utils/graph-visualizer.ts (1)
src/traverse/types.ts (1)
TraversedNode(5-12)
src/index.ts (1)
src/input-handler.ts (2)
InputOptions(5-10)createSourceFileFromInput(64-101)
🔇 Additional comments (2)
tests/traverse/interface-inheritance-dependency.test.ts (1)
20-58: Good coverage for base, multiple, and nested interface inheritance.Assertions validate membership and ordering correctly.
src/index.ts (1)
58-69: API addition looks good.
generateVisualizationcleanly reuses traversal and exposes options.
| if (Node.isTypeQuery(node)) { | ||
| const exprName = node.getExprName() | ||
|
|
||
| if (Node.isIdentifier(exprName) || Node.isQualifiedName(exprName)) { | ||
| const typeName = exprName.getText() | ||
|
|
||
| for (const qualifiedName of this.nodeGraph.nodes()) { | ||
| const nodeData = this.nodeGraph.getNode(qualifiedName) | ||
| if (nodeData.originalName === typeName) { | ||
| references.push(qualifiedName) | ||
| break | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Normalize typeof references.
exprName.getText() may be ns.Symbol; strip qualification to match originalName.
- if (Node.isIdentifier(exprName) || Node.isQualifiedName(exprName)) {
- const typeName = exprName.getText()
+ if (Node.isIdentifier(exprName) || Node.isQualifiedName(exprName)) {
+ const typeName = exprName.getText().split('.').pop()!📝 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 (Node.isTypeQuery(node)) { | |
| const exprName = node.getExprName() | |
| if (Node.isIdentifier(exprName) || Node.isQualifiedName(exprName)) { | |
| const typeName = exprName.getText() | |
| for (const qualifiedName of this.nodeGraph.nodes()) { | |
| const nodeData = this.nodeGraph.getNode(qualifiedName) | |
| if (nodeData.originalName === typeName) { | |
| references.push(qualifiedName) | |
| break | |
| } | |
| } | |
| } | |
| } | |
| if (Node.isTypeQuery(node)) { | |
| const exprName = node.getExprName() | |
| if (Node.isIdentifier(exprName) || Node.isQualifiedName(exprName)) { | |
| // Strip namespace qualification so it matches nodeData.originalName | |
| const typeName = exprName.getText().split('.').pop()! | |
| for (const qualifiedName of this.nodeGraph.nodes()) { | |
| const nodeData = this.nodeGraph.getNode(qualifiedName) | |
| if (nodeData.originalName === typeName) { | |
| references.push(qualifiedName) | |
| break | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/traverse/dependency-traversal.ts around lines 281-295, the
typeof/typeQuery handling compares exprName.getText() directly to
nodeData.originalName which fails for qualified names like "ns.Symbol";
normalize by extracting the unqualified identifier before comparing (e.g. take
the last segment after the final "." or the right-most part of a QualifiedName)
so that "ns.Symbol" becomes "Symbol", then use that normalized typeName to match
nodeData.originalName and push the qualifiedName when equal.
| <script> | ||
| // Wait for all libraries to load | ||
| window.addEventListener('load', function() { | ||
| // Use global variables from UMD builds | ||
| const Graph = window.graphology.Graph; | ||
| const Sigma = window.Sigma; | ||
| // Create a new graph instance | ||
| const graph = new Graph.DirectedGraph(); | ||
There was a problem hiding this comment.
Fix Graphology UMD usage (DirectedGraph constructor).
new Graph.DirectedGraph() is incorrect; DirectedGraph is a separate export on window.graphology.
- const Graph = window.graphology.Graph;
- const Sigma = window.Sigma;
-
- // Create a new graph instance
- const graph = new Graph.DirectedGraph();
+ const { DirectedGraph } = window.graphology;
+ const Sigma = window.Sigma;
+ // Create a new graph instance
+ const graph = new DirectedGraph();📝 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.
| <script> | |
| // Wait for all libraries to load | |
| window.addEventListener('load', function() { | |
| // Use global variables from UMD builds | |
| const Graph = window.graphology.Graph; | |
| const Sigma = window.Sigma; | |
| // Create a new graph instance | |
| const graph = new Graph.DirectedGraph(); | |
| <script> | |
| // Wait for all libraries to load | |
| window.addEventListener('load', function() { | |
| // Use global variables from UMD builds | |
| const { DirectedGraph } = window.graphology; | |
| const Sigma = window.Sigma; | |
| // Create a new graph instance | |
| const graph = new DirectedGraph(); |
🤖 Prompt for AI Agents
In src/utils/graph-visualizer.ts around lines 316 to 325, the code uses new
Graph.DirectedGraph() which is incorrect for the Graphology UMD build because
DirectedGraph is a top-level export; replace the current usage by grabbing
DirectedGraph from window.graphology (e.g., const DirectedGraph =
window.graphology.DirectedGraph) and instantiate the graph with new
DirectedGraph(), keeping Sigma and any other Graphology exports referenced
separately.
This commit introduces several improvements to the dependency traversal
logic, specifically handling interface inheritance scenarios:
interfaces, ensuring the correct order of dependencies.
multiple other interfaces.
interface that extends another interface.
These changes ensure that the dependency traversal algorithm can
accurately identify and order the dependencies for complex interface
inheritance structures, which is crucial for generating correct code.