feat(graph): rework dependency traversal with graphology DAG #13
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughReplaces the previous ts-morph codegen with a new entry at Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Index as src/index.ts
participant Input as createSourceFileFromInput
participant Traversal as DependencyTraversal
participant Printer as TypeBoxPrinter
participant TSPrint as TypeScriptPrinter
Caller->>Index: generateCode({ sourceCode, filePath, ... })
Index->>Input: createSourceFileFromInput(...)
Input-->>Index: SourceFile (main)
Index->>Traversal: startTraversal(mainSourceFile)
Traversal->>Traversal: addLocalTypes(main, isMainCode=true)
Traversal->>Traversal: collectFromImports(...) / add files to FileGraph
Traversal->>Traversal: addTypeNode(...) to NodeGraph and addDependency(...)
Traversal->>Traversal: extractDependencies() -> topological order
Traversal-->>Index: TraversedNode[] (ordered)
loop For each TraversedNode
Index->>Printer: printNode(traversedNode)
Printer->>TSPrint: emit AST into newSourceFile
TSPrint-->>Printer: updated AST/text
end
Index-->>Caller: final generated code (string)
sequenceDiagram
autonumber
participant T as DependencyTraversal
participant FG as FileGraph
participant NG as NodeGraph
participant TS as ts-morph
T->>TS: read sourceFile declarations & imports
TS-->>T: declarations, import declarations
T->>NG: addTypeNode(qualifiedName, TraversedNode)
T->>FG: addFile(filePath, sourceFile) for imports
T->>T: recurse into imported files
T->>NG: addDependency(fromQualified, toQualified)
T->>NG: removeUnusedImportedNodes()
T-->>T: getNodesToPrint() via topological sort (cycle-safe fallback)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 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: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/parsers/parse-interfaces.ts (2)
113-127: Fix invalidtypeof A<T>in generic alias emission (TS syntax error).Type queries cannot take type arguments.
typeof ${name}<T>is invalid and will fail the TS compiler. Buildtypeof ${name}without type args and rely on the alias’ own generic parameters.Apply this diff:
- // Create typeof A<T> expression - we need to create a type reference with type arguments - const typeReferenceWithArgs = ts.factory.createTypeReferenceNode( - ts.factory.createIdentifier(name), - typeArguments, - ) - - const typeofExpression = ts.factory.createTypeQueryNode( - typeReferenceWithArgs.typeName, - typeReferenceWithArgs.typeArguments, - ) + // Create typeof A expression (TS does not allow 'typeof A<T>') + const typeofExpression = ts.factory.createTypeQueryNode( + ts.factory.createIdentifier(name), + /* typeArguments */ undefined, + )Note: With this change, ensure downstream usage expects
export type A<T> = Static<ReturnType<typeof A>>. If you need tighter coupling toT, add tests for generic interfaces to validate behavior.
61-87: Generic interface path: add coverage.There’s no test asserting the generic interface emission (value + type alias) after this refactor. Add at least one case like
interface Box<T> { value: T }to guard against regressions in the new alias logic.src/traverse/dependency-traversal.ts (1)
272-301: Resolve type references by symbol (handles aliases, duplicates) and traverse type arguments.Name-only matching picks the first node with the same simple name and misses nested generics. Use symbol resolution + continue traversal to include type args. Also de-duplicate via Set.
Apply this diff:
- private extractTypeReferences(typeNode: Node): string[] { - const references: string[] = [] - const visited = new Set<Node>() + private extractTypeReferences(typeNode: Node): string[] { + const references = new Set<string>() + const visited = new Set<Node>() const traverse = (node: Node): void => { if (visited.has(node)) return visited.add(node) if (Node.isTypeReference(node)) { - const typeRefNode = node as TypeReferenceNode - const typeName = typeRefNode.getTypeName().getText() - - for (const qualifiedName of this.nodeGraph.nodes()) { - const nodeData = this.nodeGraph.getNode(qualifiedName) - if (nodeData.originalName === typeName) { - references.push(qualifiedName) - break - } - } - - return + const typeRefNode = node as TypeReferenceNode + // Prefer symbol-based resolution (handles aliases and duplicates) + const symbol = + typeRefNode.getTypeName().getSymbol() ?? typeRefNode.getType().getSymbol() + const decl = + symbol?.getDeclarations()?.find((d) => + Node.isTypeAliasDeclaration(d) || + Node.isInterfaceDeclaration(d) || + Node.isEnumDeclaration(d) || + Node.isFunctionDeclaration(d), + ) + + if (decl && decl.getName && decl.getSourceFile) { + const qn = generateQualifiedNodeName(decl.getName()!, decl.getSourceFile()) + if (this.nodeGraph.hasNode(qn)) references.add(qn) + } else { + // Fallback: simple-name match (best effort) + const typeName = typeRefNode.getTypeName().getText() + for (const qualifiedName of this.nodeGraph.nodes()) { + const nodeData = this.nodeGraph.getNode(qualifiedName) + if (nodeData.originalName === typeName) { + references.add(qualifiedName) + break + } + } + } + // Continue into type arguments to capture nested references + node.forEachChild(traverse) + return } node.forEachChild(traverse) } traverse(typeNode) - return references + return Array.from(references) }
🧹 Nitpick comments (25)
package.json (1)
4-5: TS files as "main"/"module" can be brittle; prefer "exports" or compiled entry.Given "private": true and Bun usage, this may work, but for portability consider adding an "exports" map and/or emitting JS for Node consumers.
Proposed minimal tweak:
"name": "@daxserver/validation-schema-codegen", "version": "0.1.0", - "main": "src/index.ts", - "module": "src/index.ts", + "main": "src/index.ts", + "module": "src/index.ts", + "exports": { + ".": { + "types": "./src/index.ts", + "import": "./src/index.ts" + } + },If you intend Node consumption, switch to built JS and point exports/import/types accordingly.
src/utils/generate-qualified-name.ts (1)
1-1: Use node: specifier to avoid polyfills under bundler resolution.-import { basename } from 'path' +import { basename } from 'node:path'src/traverse/file-graph.ts (1)
8-19: Add typing for node attributes to catch mistakes at compile time.-import { DirectedGraph } from 'graphology' -import type { SourceFile } from 'ts-morph' +import { DirectedGraph } from 'graphology' +import type { SourceFile } from 'ts-morph' +import type { Attributes } from 'graphology-types' + +type FileNodeAttributes = { + type: 'file' + sourceFile: SourceFile +} -export class FileGraph extends DirectedGraph { +export class FileGraph extends DirectedGraph<FileNodeAttributes, Attributes, Attributes> { /** * Add a file to the graph */ addFile(filePath: string, sourceFile: SourceFile): void { if (this.hasNode(filePath)) return - this.addNode(filePath, { + this.addNode(filePath, { type: 'file', sourceFile, }) } }tests/handlers/typebox/functions.test.ts (1)
12-181: Reduce duplication with a small helper for common patterns.Example:
const expectCode = (project: Project, ts: string, out: string) => { const sf = createSourceFile(project, ts) return expect(generateFormattedCode(sf)).resolves.toBe(formatWithPrettier(out)) }Then reuse in each test.
src/parsers/base-parser.ts (1)
10-12: Prefer readonly fields for parser dependenciesThese are set once in the constructor and should not be reassigned by subclasses. Mark them readonly for tighter invariants.
Apply:
- protected newSourceFile: SourceFile - protected printer: ts.Printer - protected processedTypes: Set<string> + protected readonly newSourceFile: SourceFile + protected readonly printer: ts.Printer + protected readonly processedTypes: Set<string>Also applies to: 15-18
src/traverse/dependency-graph.ts (2)
5-7: Small API ergonomics: chainable add and typed getterReturn
thisfor chaining and provide a typedgetwrapper to avoid casts at call sites.export class DependencyGraph extends DirectedGraph { - add(name: string, node: TraversedNode) { - this.addNode(name, node) - } + add(name: string, node: TraversedNode): this { + this.addNode(name, node) + return this + } + + get(name: string): TraversedNode | undefined { + return this.hasNode(name) ? (this.getNodeAttributes(name) as TraversedNode) : undefined + } }
1-8: Remove unusedDependencyGraphor wire it in: TheDependencyGraphclass insrc/traverse/dependency-graph.tsisn’t referenced anywhere else; remove it to prevent bitrot or integrate it where needed.tests/utils.ts (1)
26-37: Prettier harness is solid; one minor: derive TSchema import automaticallyOptional: infer
withTSchemaby scanning the generated code for<T extends TSchema>to avoid repeating the signal in each test expectation.-export const generateFormattedCode = async ( - sourceFile: SourceFile, - withTSchema: boolean = false, -): Promise<string> => { +export const generateFormattedCode = async ( + sourceFile: SourceFile, + withTSchema?: boolean, +): Promise<string> => { const code = await generateCode({ sourceCode: sourceFile.getFullText(), callerFile: sourceFile.getFilePath(), project: sourceFile.getProject(), }) - return formatWithPrettier(code, false, withTSchema) + const needsTSchema = withTSchema ?? /<\s*T\s+extends\s+TSchema\s*>/m.test(code) + return formatWithPrettier(code, false, needsTSchema) }tests/handlers/typebox/typeof.test.ts (1)
12-12: Rename context label to avoid confusionConsider renaming “without exports” to “input without exports” since the expectations are exported artifacts now.
tests/handlers/typebox/enums.test.ts (1)
12-12: Rename context label to reflect input, not outputChange “without exports” to “input without exports” for clarity.
tests/handlers/typebox/objects.test.ts (1)
12-12: Nit: clarify context labelRename “without export” → “input without export” to avoid ambiguity.
src/utils/add-static-type-alias.ts (1)
4-9: Document and consider controllable export for special casesAdd TSDoc noting the unconditional export. Consider an optional parameter for future callers that need non-exported aliases (e.g., internal scaffolding), defaulting to true.
export const addStaticTypeAlias = ( newSourceFile: SourceFile, name: string, compilerNode: ts.SourceFile, printer: ts.Printer, ) => { + // Emits `export type <name> = Static<typeof <name>>;` + // Note: Export is unconditional by design to align with the public API policy.tests/handlers/typebox/template-literals.test.ts (1)
12-12: Clarify describe titles to reflect intentRename to avoid confusion between input and output: they currently say “without exports” yet expect exported output.
- describe('without exports', () => { + describe('input: non-exported type aliases', () => {And:
- describe('with exports', () => { + describe('input: exported type aliases', () => {src/parsers/parse-function-declarations.ts (1)
33-51: Ignore TS ‘this’ parameter when generating Type.Function argumentsThe fake
thisparameter (if present) should not become a runtime argument schema.- const parameterTypes = parameters.map((param) => { + const parameterTypes = parameters + .filter((p) => p.getName() !== 'this') + .map((param) => {src/printer/typebox-printer.ts (3)
38-60: Leverage qualified names to avoid cross-file collisionsParsers share
processedTypeskeyed by name. SinceTraversedNodehasqualifiedName, consider:
- Change
processedTypesto trackqualifiedName.- Pass
traversedNode(not justnode) to parser methods, or add aprocessedKeyparameter.
This prevents silent drops when different modules export same-named types.If you want minimal churn now, set the key to
node.getSourceFile().getFilePath() + '#' + namein each parser.
38-41:isImportedparam is unusedDrop it from the
printNodesignature to reduce noise, or start using it if you plan different behaviors.- printNode(traversedNode: { node: Node; isImported?: boolean }): void { + printNode(traversedNode: { node: Node }): void {
58-59: Optional: handle unknown node kinds explicitlyConsider logging a debug message to ease diagnosing missed kinds.
src/index.ts (1)
56-84: Optional:generateCodedoesn’t need to be asyncNo
awaitis used; you could return a string directly unless other callers rely on a Promise.tests/handlers/typebox/arrays.test.ts (1)
12-12: Rename misleading describe label.These tests intentionally expect exported bindings even when the source lacks exports. Rename to avoid confusion.
- describe('without export', () => { + describe('source without export declarations', () => {tests/handlers/typebox/primitive-types.test.ts (1)
12-12: Rename misleading describe label.Mirror the suite-wide convention and avoid implying the output is non-exported.
- describe('without exports', () => { + describe('source without export declarations', () => {src/parsers/parse-enums.ts (1)
18-20: Prefer AST-based initializer for consistency.Everywhere else you print via
ts.Printer. Consider buildingType.Enum(Identifier(enumName))as an AST node and printing it, avoiding template strings.tests/traverse/dependency-collector.integration.test.ts (2)
11-11: Rename suite to reflect new subject.The suite is still named “DependencyCollector” though it exercises
DependencyTraversal. Rename for clarity.Apply this diff:
-describe('DependencyCollector', () => { +describe('DependencyTraversal', () => {
156-172: Alias imports: add an assertion that alias resolves to original declaration in edges.Current test checks de-duplication but not that
User as UserAliasresolves dependencies toUser. Add a property using the alias and assert topo order or edge presence.Example (add a new test):
test('should resolve alias import to original declaration', () => { createSourceFile(project, `export type User = { id: string }`, 'user.ts') const mainFile = createSourceFile(project, ` import { User as UserAlias } from "./user"; type Local = { u: UserAlias } `, 'main.ts') traverser.addLocalTypes(mainFile) traverser.collectFromImports(mainFile.getImportDeclarations(), true, mainFile) traverser.extractDependencies() const deps = traverser.getNodesToPrint().map(getNodeName) expect(deps).toEqual(['User', 'Local']) })src/traverse/dependency-traversal.ts (2)
164-165: Unused parametermainSourceFile.
collectFromImportsnever usesmainSourceFile. Either remove it or prefix with_to signal intentional disregard.Apply this diff (minimal signaling):
-collectFromImports(importDeclarations: ImportDeclaration[], isDirectImport: boolean = true, mainSourceFile?: SourceFile): void { +collectFromImports(importDeclarations: ImportDeclaration[], isDirectImport: boolean = true, _mainSourceFile?: SourceFile): void {
144-159: Remove dead code:isTypeUsedInSourceFileis unused.Drop this helper to reduce noise, or wire it where intended.
📜 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 selected for processing (37)
ARCHITECTURE.md(2 hunks)package.json(1 hunks)src/index.ts(1 hunks)src/parsers/base-parser.ts(1 hunks)src/parsers/parse-enums.ts(1 hunks)src/parsers/parse-function-declarations.ts(2 hunks)src/parsers/parse-interfaces.ts(6 hunks)src/parsers/parse-type-aliases.ts(2 hunks)src/printer/typebox-printer.ts(1 hunks)src/traverse/dependency-file-resolver.ts(0 hunks)src/traverse/dependency-graph.ts(1 hunks)src/traverse/dependency-traversal.ts(2 hunks)src/traverse/file-graph.ts(1 hunks)src/traverse/node-graph.ts(1 hunks)src/traverse/types.ts(1 hunks)src/ts-morph-codegen.ts(0 hunks)src/utils/add-static-type-alias.ts(1 hunks)src/utils/generate-qualified-name.ts(1 hunks)tests/export-everything.test.ts(0 hunks)tests/handlers/typebox/advanced-types.test.ts(0 hunks)tests/handlers/typebox/arrays.test.ts(7 hunks)tests/handlers/typebox/enum-types.test.ts(0 hunks)tests/handlers/typebox/enums.test.ts(1 hunks)tests/handlers/typebox/functions.test.ts(12 hunks)tests/handlers/typebox/interfaces.test.ts(8 hunks)tests/handlers/typebox/objects.test.ts(2 hunks)tests/handlers/typebox/primitive-types.test.ts(14 hunks)tests/handlers/typebox/template-literal-types.test.ts(0 hunks)tests/handlers/typebox/template-literals.test.ts(1 hunks)tests/handlers/typebox/typeof.test.ts(1 hunks)tests/handlers/typebox/utility-types.test.ts(8 hunks)tests/import-resolution.test.ts(8 hunks)tests/traverse/dependency-collector.integration.test.ts(1 hunks)tests/traverse/dependency-collector.unit.test.ts(0 hunks)tests/traverse/dependency-ordering.test.ts(1 hunks)tests/utils.ts(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (7)
- tests/handlers/typebox/enum-types.test.ts
- tests/traverse/dependency-collector.unit.test.ts
- tests/handlers/typebox/template-literal-types.test.ts
- tests/export-everything.test.ts
- src/ts-morph-codegen.ts
- src/traverse/dependency-file-resolver.ts
- tests/handlers/typebox/advanced-types.test.ts
🧰 Additional context used
🧬 Code graph analysis (14)
tests/handlers/typebox/enums.test.ts (2)
tests/utils.ts (3)
createSourceFile(12-14)generateFormattedCode(26-37)formatWithPrettier(16-24)tests/handlers/typebox/enum-types.test.ts (6)
test(12-58)project(5-107)sourceFile(13-34)sourceFile(84-105)sourceFile(36-57)sourceFile(61-82)
src/printer/typebox-printer.ts (4)
src/parsers/parse-type-aliases.ts (1)
TypeAliasParser(7-41)src/parsers/parse-interfaces.ts (1)
InterfaceParser(11-149)src/parsers/parse-enums.ts (1)
EnumParser(5-39)src/parsers/parse-function-declarations.ts (1)
FunctionDeclarationParser(7-90)
src/traverse/node-graph.ts (1)
src/traverse/types.ts (1)
TraversedNode(10-19)
src/index.ts (4)
src/traverse/types.ts (1)
TraversedNode(10-19)src/printer/typebox-printer.ts (1)
TypeBoxPrinter(12-61)src/input-handler.ts (2)
InputOptions(5-10)createSourceFileFromInput(64-101)src/traverse/dependency-traversal.ts (1)
DependencyTraversal(12-302)
tests/handlers/typebox/template-literals.test.ts (2)
tests/utils.ts (3)
createSourceFile(12-14)generateFormattedCode(26-37)formatWithPrettier(16-24)tests/handlers/typebox/template-literal-types.test.ts (3)
project(5-95)sourceFile(80-94)sourceFile(24-36)
src/traverse/types.ts (1)
src/traverse/node-graph.ts (1)
NodeGraph(8-57)
tests/handlers/typebox/typeof.test.ts (2)
tests/utils.ts (3)
createSourceFile(12-14)generateFormattedCode(26-37)formatWithPrettier(16-24)tests/handlers/typebox/advanced-types.test.ts (1)
test(12-50)
tests/traverse/dependency-collector.integration.test.ts (3)
src/traverse/types.ts (1)
TraversedNode(10-19)src/traverse/dependency-traversal.ts (1)
DependencyTraversal(12-302)tests/utils.ts (1)
createSourceFile(12-14)
tests/traverse/dependency-ordering.test.ts (1)
tests/utils.ts (2)
generateFormattedCode(26-37)formatWithPrettier(16-24)
src/parsers/parse-enums.ts (1)
src/utils/add-static-type-alias.ts (1)
addStaticTypeAlias(4-22)
tests/handlers/typebox/primitive-types.test.ts (1)
tests/utils.ts (1)
generateFormattedCode(26-37)
tests/handlers/typebox/functions.test.ts (2)
tests/utils.ts (1)
generateFormattedCode(26-37)tests/handlers/typebox/function-types.test.ts (2)
sourceFile(14-24)sourceFile(52-62)
src/traverse/dependency-graph.ts (1)
src/traverse/types.ts (1)
TraversedNode(10-19)
src/traverse/dependency-traversal.ts (4)
src/traverse/file-graph.ts (1)
FileGraph(8-20)src/traverse/node-graph.ts (1)
NodeGraph(8-57)src/traverse/types.ts (1)
TraversedNode(10-19)src/utils/generate-qualified-name.ts (1)
generateQualifiedNodeName(20-25)
🪛 LanguageTool
ARCHITECTURE.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...cture ## Table of Contents - Overview - Core Components - [...
(QB_NEW_EN)
[grammar] ~6-~6: There might be a mistake here.
Context: ...Overview - Core Components - [Code Generation Flow](#code-generation-f...
(QB_NEW_EN)
[grammar] ~7-~7: There might be a mistake here.
Context: ...re-components) - Code Generation Flow - TypeBox Printer - [...
(QB_NEW_EN)
[grammar] ~8-~8: There might be a mistake here.
Context: ...de-generation-flow) - TypeBox Printer - [Dependency Management](#dependency-manag...
(QB_NEW_EN)
[grammar] ~9-~9: There might be a mistake here.
Context: ...ebox-printer) - Dependency Management - Parser System - [Hand...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...ependency-management) - Parser System - Handler System - [Im...
(QB_NEW_EN)
[grammar] ~11-~11: There might be a mistake here.
Context: ...tem](#parser-system) - Handler System - Import Resolution ...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...(#handler-system) - Import Resolution - Input Handling - [In...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...(#import-resolution) - Input Handling - [Interface Inheritance](#interface-inheri...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...put-handling) - Interface Inheritance - Utility Functions -...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...face-inheritance) - Utility Functions - Process Overview - [...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...(#utility-functions) - Process Overview - Basic Usage - [Testing](#...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...rview](#process-overview) - Basic Usage - Testing ## Overview `@daxse...
(QB_NEW_EN)
[grammar] ~26-~26: There might be a mistake here.
Context: ...rfaces, enums, and function declarations - Generic Types: Generic interfaces and ...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ...rameters with proper constraint handling - Complex Types: Union and intersection ...
(QB_NEW_EN)
[grammar] ~28-~28: There might be a mistake here.
Context: ...bject structures, template literal types - Utility Types: Built-in support for Pi...
(QB_NEW_EN)
[grammar] ~29-~29: There might be a mistake here.
Context: ...cord, and other TypeScript utility types - Advanced Features: Conditional types, ...
(QB_NEW_EN)
[grammar] ~30-~30: There might be a mistake here.
Context: ...s, keyof operators, indexed access types - Import Resolution: Cross-file type dep...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...Creates a SourceFile from input using createSourceFileFromInput 2. Generic Interface Detection: Checks fo...
(QB_NEW_EN)
[grammar] ~43-~43: There might be a mistake here.
Context: ...ssary @sinclair/typebox imports using createOutputFile 4. Dependency Traversal: Uses `Dependency...
(QB_NEW_EN)
[grammar] ~45-~45: There might be a mistake here.
Context: ... sorted nodes using TypeBoxPrinter in printSortedNodes 6. Output Formatting: Returns the formatt...
(QB_NEW_EN)
[grammar] ~65-~65: There might be a mistake here.
Context: ...versal.ts"> - Main orchestrator 2. FileGraph: <mcfile name="file-graph.ts...
(QB_NEW_EN)
[grammar] ~66-~66: There might be a mistake here.
Context: ...mcfile> - Tracks file-level dependencies 3. NodeGraph: <mcfile name="node-graph.ts...
(QB_NEW_EN)
[grammar] ~71-~71: There might be a mistake here.
Context: ...Adds all types from the main source file 2. Import Processing: Recursively process...
(QB_NEW_EN)
[grammar] ~72-~72: There might be a mistake here.
Context: ...ecursively processes import declarations 3. Dependency Extraction: Analyzes type r...
(QB_NEW_EN)
[grammar] ~73-~73: There might be a mistake here.
Context: ...ype references to build dependency graph 4. Topological Sorting: Returns nodes in ...
(QB_NEW_EN)
[grammar] ~82-~82: There might be a mistake here.
Context: ...sers/base-parser.ts"> provides: - Common interface for all parsers - Share...
(QB_NEW_EN)
[grammar] ~83-~83: There might be a mistake here.
Context: ...ides: - Common interface for all parsers - Shared access to output SourceFile, Ty...
(QB_NEW_EN)
[grammar] ~84-~84: There might be a mistake here.
Context: ...pt printer, and processed types tracking - Abstract parse method for implementati...
(QB_NEW_EN)
[grammar] ~89-~89: There might be a mistake here.
Context: ...dles both regular and generic interfaces 2. TypeAliasParser: <mcfile name="parse-t...
(QB_NEW_EN)
[grammar] ~90-~90: There might be a mistake here.
Context: ...ile> - Processes type alias declarations 3. EnumParser: <mcfile name="parse-enums....
(QB_NEW_EN)
[grammar] ~91-~91: There might be a mistake here.
Context: ...s"> - Handles enum declarations 4. FunctionParser: <mcfile name="parse-fu...
(QB_NEW_EN)
[grammar] ~92-~92: There might be a mistake here.
Context: ...cfile> - Processes function declarations 5. ImportParser: <mcfile name="parse-impo...
(QB_NEW_EN)
[grammar] ~113-~113: There might be a mistake here.
Context: ...class orchestrates all handlers through: - Handler Caching: Caches handler instan...
(QB_NEW_EN)
[grammar] ~121-~121: There might be a mistake here.
Context: ...pports nested import chains of any depth - Circular Import Detection: Prevents in...
(QB_NEW_EN)
[grammar] ~122-~122: There might be a mistake here.
Context: ...Prevents infinite loops during traversal - Qualified Naming: Resolves type name c...
(QB_NEW_EN)
[grammar] ~144-~144: There might be a mistake here.
Context: ...either file paths or source code strings 2. Path Resolution: Handles both absolute...
(QB_NEW_EN)
[grammar] ~145-~145: There might be a mistake here.
Context: ...lative file paths with proper validation 3. Relative Import Validation: Prevents r...
(QB_NEW_EN)
[grammar] ~146-~146: There might be a mistake here.
Context: ...nless a callerFile context is provided 4. Project Context Sharing: Supports pass...
(QB_NEW_EN)
[grammar] ~147-~147: There might be a mistake here.
Context: ...es to maintain import resolution context 5. Error Handling: Provides clear error m...
(QB_NEW_EN)
[grammar] ~190-~190: There might be a mistake here.
Context: ...resenting the equivalent TypeBox schema. - : Generates and adds the `export type [T...
(QB_NEW_EN)
[grammar] ~213-~213: There might be a mistake here.
Context: ...endency collection and ordering process: - Collects local types from the main sourc...
(QB_NEW_EN)
[grammar] ~218-~218: There might be a mistake here.
Context: ...cies are handled before dependent types. 5. TypeBox Schema Generation: For each no...
(QB_NEW_EN)
[grammar] ~244-~244: There might be a mistake here.
Context: ...onent testing using Bun's test framework - Integration Tests: End-to-end testing ...
(QB_NEW_EN)
[grammar] ~245-~245: There might be a mistake here.
Context: ...ting of the complete generation pipeline - Type Safety Tests: Validation of gener...
(QB_NEW_EN)
[grammar] ~246-~246: There might be a mistake here.
Context: ... Validation of generated TypeBox schemas - Edge Case Coverage: Testing of complex...
(QB_NEW_EN)
[grammar] ~251-~251: There might be a mistake here.
Context: ...peScript Compiler**: Type checking with bun tsc - ESLint: Code quality and style enforce...
(QB_NEW_EN)
[grammar] ~252-~252: There might be a mistake here.
Context: ...nt**: Code quality and style enforcement - Prettier: Consistent code formatting -...
(QB_NEW_EN)
[grammar] ~253-~253: There might be a mistake here.
Context: ...Prettier: Consistent code formatting - Bun Test: Fast and reliable test execu...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
ARCHITECTURE.md
214-214: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
215-215: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
216-216: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
217-217: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
🔇 Additional comments (20)
src/utils/generate-qualified-name.ts (1)
20-25: Confirm determinism: absolute paths in hash can make outputs machine/path-dependent.If the qualified name is emitted (even indirectly), prefer hashing a project-relative path.
Option (if needed): pass a base path and hash
relative(base, filePath).tests/handlers/typebox/functions.test.ts (1)
19-21: LGTM: expected outputs now consistently export TypeBox value and Static type.Also applies to: 31-33, 43-45, 103-106, 118-121, 130-133
tests/handlers/typebox/interfaces.test.ts (2)
18-23: LGTM: export semantics updated consistentlyThe expectations now require exported runtime schemas and exported
Staticaliases across basic, inherited, and generic cases. This aligns with the “always-export” parser changes.Also applies to: 54-69, 84-105, 149-161, 177-200, 217-228, 247-258, 280-296
221-222: Confirmedtypeof A<T>support—no changes needed
Tested with TypeScript 5.9.2:ReturnType<typeof A<T>>compiles under--strict/--noEmit.tests/utils.ts (1)
1-1: LGTM: switched tests to new public entrypointImporting
generateCodefrom the package root reflects the newsrc/index.tsentry and keeps tests aligned with the public API.tests/handlers/typebox/typeof.test.ts (1)
1-11: typeof export semantics are consistent across test suites
No legacy advanced-types suite found; typebox tests intentionally cover both non-exported and exportedtypeofcases.tests/handlers/typebox/objects.test.ts (1)
18-23: LGTM: export surface matches new policyExporting both the runtime schema and Static alias aligns with the unified policy.
Also applies to: 32-35
src/utils/add-static-type-alias.ts (2)
17-21: Forced export matches new policyHard-coding
isExported: truealigns with the suite-wide “always export” stance.
1-3: All addStaticTypeAlias call sites updated
No callers still pass the removedisExportedparameter.src/traverse/types.ts (1)
8-19: Types look good and align with NodeGraph usageThe
TraversedNodeshape matches NodeGraph expectations and supports DAG traversal flags.src/parsers/parse-function-declarations.ts (2)
72-88: Always-export change is consistent with the new pipelineFlag aligns with other parsers; OK.
24-28: Name-based deduping currently safe. No duplicate top-level function/type/interface names were found in src; using bare names in processedTypes poses no immediate risk.src/index.ts (1)
39-54: Printer wiring looks solidTopologically ordered printing via TypeBoxPrinter is straightforward and cohesive.
tests/handlers/typebox/arrays.test.ts (1)
123-231: LGTM: exported expectations match new pipeline.The “with export” block aligns with the new always-exported emission and the updated test harness.
tests/handlers/typebox/primitive-types.test.ts (1)
102-107: LGTM: harness callsite and expectations are consistent.Dropping the extra boolean from
generateFormattedCodeand expecting exported bindings look correct.Also applies to: 114-119, 126-131, 138-143, 150-155, 162-167, 174-179
src/parsers/parse-type-aliases.ts (1)
23-39: LGTM: simplified, always-exported emission.Unifying on exported const + static alias aligns with test expectations and the updated helper signature.
src/parsers/parse-interfaces.ts (2)
97-97: VerifyTSchemaimport is present in generated files.You constrain type params with
TSchemabut this file doesn’t add/import it. Confirm the codegen ensuresTSchemais imported from@sinclair/typeboxwherever this alias is emitted; otherwise consumers will get missing identifier errors.
42-58: Always-export policy: LGTM.Consistent
isExported: truefor values and static aliases improves DX and aligns with updated tests.tests/import-resolution.test.ts (1)
40-54: Updated export-surface expectations: LGTM.The assertions now match the “always export” policy for both value and type aliases across nested/package imports.
Also applies to: 85-99, 141-161, 215-241, 274-289, 319-334, 375-394, 449-475
tests/handlers/typebox/utility-types.test.ts (1)
12-34: Utility-types export policy: LGTM.Consistent exported
const+typeacross KeyOf/Record/Partial/Pick/Omit/Required/Indexed Access aligns with the new API.Also applies to: 41-46, 55-63, 72-81, 90-99, 108-116, 133-142, 147-170, 176-181, 189-198, 205-216, 223-234, 243-251, 269-278
| import { formatWithPrettier, generateFormattedCode } from '@test-fixtures/utils' | ||
| import { beforeEach, describe, expect, test } from 'bun:test' | ||
| import { Project } from 'ts-morph' | ||
|
|
||
| describe('Dependency Ordering Bug', () => { | ||
| let project: Project | ||
|
|
||
| beforeEach(() => { | ||
| project = new Project() | ||
| }) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Search for other instances of quoted-in-quoted property keys
There may be more cases where string-literal keys are emitted with embedded quotes.
Run:
🏁 Script executed:
#!/bin/bash
rg -nP -C1 "['\"]'[^'\"]+['\"]\s*:" testsLength of output: 52
🏁 Script executed:
#!/bin/bash
# Search for double-quoted keys containing single quotes
rg -n -C1 "\"'.*'\":\s*" tests || true
# Search for single-quoted keys containing double quotes
rg -n -C1 "'\".*\"':\s*" tests || trueLength of output: 628
Fix extra quotes in test property keys
In tests/traverse/dependency-ordering.test.ts (lines 82–85), the DataValueByDataType object uses keys with embedded quotes (e.g. "'string'" instead of "string"). Remove the inner quotes so the keys are the raw data type names.
🤖 Prompt for AI Agents
In tests/traverse/dependency-ordering.test.ts around lines 82 to 85, the
DataValueByDataType object currently uses keys with embedded quotes (e.g.
"'string'") which is incorrect; update the object so the keys are unquoted raw
data type names (e.g. string instead of 'string') by removing the inner quotes
from each key, leaving the values unchanged and ensuring the object uses valid
identifier or string keys as intended.
| export const DataValueByDataType = Type.Object({ | ||
| "'string'": StringSnakDataValue, | ||
| "'commonsMedia'": CommonsMediaSnakDataValue, | ||
| "'external-id'": ExternalIdSnakDataValue, | ||
| "'geo-shape'": GeoShapeSnakDataValue, | ||
| }); | ||
| export type DataValueByDataType = Static<typeof DataValueByDataType>; | ||
| `), |
There was a problem hiding this comment.
Incorrect double-quoted property keys in runtime object
Keys like "'string'" include quotes as part of the key. They should be normal string keys: 'string', 'commonsMedia', 'external-id', 'geo-shape'.
Apply:
- export const DataValueByDataType = Type.Object({
- "'string'": StringSnakDataValue,
- "'commonsMedia'": CommonsMediaSnakDataValue,
- "'external-id'": ExternalIdSnakDataValue,
- "'geo-shape'": GeoShapeSnakDataValue,
- });
+ export const DataValueByDataType = Type.Object({
+ 'string': StringSnakDataValue,
+ 'commonsMedia': CommonsMediaSnakDataValue,
+ 'external-id': ExternalIdSnakDataValue,
+ 'geo-shape': GeoShapeSnakDataValue,
+ });📝 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.
| export const DataValueByDataType = Type.Object({ | |
| "'string'": StringSnakDataValue, | |
| "'commonsMedia'": CommonsMediaSnakDataValue, | |
| "'external-id'": ExternalIdSnakDataValue, | |
| "'geo-shape'": GeoShapeSnakDataValue, | |
| }); | |
| export type DataValueByDataType = Static<typeof DataValueByDataType>; | |
| `), | |
| export const DataValueByDataType = Type.Object({ | |
| 'string': StringSnakDataValue, | |
| 'commonsMedia': CommonsMediaSnakDataValue, | |
| 'external-id': ExternalIdSnakDataValue, | |
| 'geo-shape': GeoShapeSnakDataValue, | |
| }); | |
| export type DataValueByDataType = Static<typeof DataValueByDataType>; |
🤖 Prompt for AI Agents
In tests/traverse/dependency-ordering.test.ts around lines 81 to 89, the runtime
object uses keys that include literal quote characters (e.g. "'string'") which
makes the property names incorrect; change those keys to normal string
identifiers without embedded quotes (e.g. 'string', 'commonsMedia',
'external-id', 'geo-shape') so the object maps the intended data type strings to
their corresponding SnakDataValue types and keep the rest of the declaration
unchanged.
| "@daxserver/validation-schema-codegen": ["./src/index.ts"], | ||
| "@daxserver/validation-schema-codegen/*": ["./src/*"], | ||
| "@test-fixtures/*": ["./tests/*"] | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure runtime aliasing for Bun/Node, not just TS type-checking.
The new path alias resolves in the TS compiler, but Bun/Node won’t honor it at runtime unless configured (bunfig.toml aliases or package.json "imports"). Verify tests run without path-resolution errors when importing "@daxserver/validation-schema-codegen" or "@test-fixtures/*".
Run:
🏁 Script executed:
#!/bin/bash
# Check bun config and runtime aliasing
fd -a 'bunfig.*' -HI
rg -n '"imports"\s*:' package.json
rg -n '@daxserver/validation-schema-codegen"|@test-fixtures/' tests --type=ts -C1Length of output: 3665
Configure runtime module aliases for Bun/Node. TS path aliases aren’t applied at runtime—add corresponding mappings in bunfig.toml (or the package.json "imports" field) for both @daxserver/validation-schema-codegen (and its subpaths) and @test-fixtures/*, then re-run tests to confirm there are no import-resolution errors.
🤖 Prompt for AI Agents
In tsconfig.json around lines 27 to 30, the TypeScript path aliases for
"@daxserver/validation-schema-codegen" (and its /* subpaths) and
"@test-fixtures/*" only affect compilation and are not available at runtime; add
equivalent runtime mappings (either in bunfig.toml under the [imports] table or
in package.json "imports") mapping "@daxserver/validation-schema-codegen" to
"./src/index.ts", "@daxserver/validation-schema-codegen/*" to "./src/*", and
"@test-fixtures/*" to "./tests/*"; save the changes and re-run the test suite to
confirm import-resolution is fixed.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/parsers/parse-function-declarations.ts (1)
20-37: Optional-parameter detection misses defaultsParameters with initializers are optional at call sites. Wrap them in Type.Optional as well.
Apply:
- if (param.hasQuestionToken()) { + if (param.hasQuestionToken() || param.hasInitializer()) { return ts.factory.createCallExpression(src/parsers/parse-interfaces.ts (1)
15-20: Same-name interface dedupe collisionprocessedTypes keyed by interfaceName will skip legitimate, same-named interfaces from different files. Use a qualified key (e.g., generateQualifiedNodeName) or accept an injected outputName.
Happy to provide a patch that threads TraversedNode.qualifiedName (or a collision-resolved name) into all parser.parse calls.
src/traverse/dependency-traversal.ts (1)
249-277: Type reference extraction misses nested generics, qualified names, and heritage nodes
- Early return after first TypeReference skips its type arguments (e.g., Array never records Foo).
- Qualified/namespace references (“ns.Foo”) won’t match originalName.
- Extends/heritage references aren’t handled.
- private extractTypeReferences(node: Node): string[] { + private extractTypeReferences(node: Node): string[] { const references: string[] = [] const visited = new Set<Node>() const traverse = (node: Node): void => { if (visited.has(node)) return visited.add(node) - if (Node.isTypeReference(node)) { - const typeRefNode = node as TypeReferenceNode - const typeName = typeRefNode.getTypeName().getText() - - for (const qualifiedName of this.nodeGraph.nodes()) { - const nodeData = this.nodeGraph.getNode(qualifiedName) - if (nodeData.originalName === typeName) { - references.push(qualifiedName) - break - } - } - - return - } + // Handle TypeReference<TArgs...> + if (Node.isTypeReference(node)) { + const typeRefNode = node as TypeReferenceNode + const typeNameEntity = typeRefNode.getTypeName() + const simpleName = + Node.isIdentifier(typeNameEntity) + ? typeNameEntity.getText() + : // QualifiedName or PropertyAccess => take rightmost identifier + (typeNameEntity as any).getRightSide?.().getText?.() ?? + typeNameEntity.getText().split('.').pop()! + + for (const qualifiedName of this.nodeGraph.nodes()) { + const nodeData = this.nodeGraph.getNode(qualifiedName) + if (nodeData.originalName === simpleName) { + references.push(qualifiedName) + break + } + } + // Also traverse type arguments to find nested refs (e.g., Array<Foo>) + for (const ta of typeRefNode.getTypeArguments()) traverse(ta) + // Do not return; continue to traverse siblings/children + } + + // Handle heritage: ExpressionWithTypeArguments (e.g., extends Base<T>) + if ((Node as any).isExpressionWithTypeArguments?.(node)) { + const expr = (node as any).getExpression?.() + const simpleName = expr?.getText?.()?.split('.').pop?.() + if (simpleName) { + for (const qn of this.nodeGraph.nodes()) { + const data = this.nodeGraph.getNode(qn) + if (data.originalName === simpleName) { + references.push(qn) + break + } + } + } + for (const ta of (node as any).getTypeArguments?.() ?? []) traverse(ta) + } node.forEachChild(traverse) } traverse(node) return references }src/parsers/parse-type-aliases.ts (1)
23-35: Name-collision risk: printing original names can clash across files.
Both theconstandexport typeusetypeAlias.getName(); if two imported files export the same alias (e.g.,User), you’ll generate duplicate declarations in the single output file. Current code has no renaming strategy or collision check.Minimal stopgap (block generation on collisions) — add this guard in
src/index.tsbefore printing:@@ const traversedNodes = dependencyTraversal.startTraversal(sourceFile) @@ const hasGenericInterfaces = traversedNodes.some( (t) => Node.isInterfaceDeclaration(t.node) && t.node.getTypeParameters().length > 0, ) + + // Guard against duplicate original names coming from different files + const seen = new Map<string, string>() // name -> qualifiedName + const collisions: string[] = [] + for (const t of traversedNodes) { + const prev = seen.get(t.originalName) + if (prev && prev !== t.qualifiedName) { + collisions.push(`${t.originalName} (${prev} vs ${t.qualifiedName})`) + } else { + seen.set(t.originalName, t.qualifiedName) + } + } + if (collisions.length) { + throw new Error( + `Name collisions detected across modules: ${collisions.join( + ', ', + )}. Implement a renaming strategy (e.g., file-qualified identifiers).`, + ) + }Longer-term: introduce a print-name registry keyed by
qualifiedNameand rewrite TypeReference identifiers during printing to those unique names. I can sketch this if you want.
♻️ Duplicate comments (5)
ARCHITECTURE.md (2)
35-35: Doc type name mismatch: UseInputOptions, notGenerateCodeOptionsApply:
-... `generateCode`, takes a `GenerateCodeOptions` object ... +... `generateCode`, takes an `InputOptions` object ...
217-221: Fix nested list indentation (MD007) under “Centralized Dependency Traversal”- - Collects local types from the main source file - - Recursively traverses import chains to gather all dependencies - - Establishes dependency relationships between all types - - Returns topologically sorted nodes for processing + - Collects local types from the main source file + - Recursively traverses import chains to gather all dependencies + - Establishes dependency relationships between all types + - Returns topologically sorted nodes for processingsrc/traverse/dependency-traversal.ts (1)
35-40: Prune unused imported nodes before sortingCall removeUnusedImportedNodes() after dependency extraction to avoid emitting unused types.
// Extract dependencies for all nodes this.extractDependencies() + // Prune unused imported nodes (keeps direct imports) + this.nodeGraph.removeUnusedImportedNodes() + // Return topologically sorted nodes return this.getNodesToPrint()tests/handlers/typebox/functions.test.ts (1)
17-23: Switched to synchronous expectations. Fix addresses prior “.resolves” issue.src/index.ts (1)
61-69: Fixed: generic-interface detection from traversed graph (not only main file).
This addresses the earlier review concern about missing TSchema imports for generics in imported files.
🧹 Nitpick comments (14)
src/parsers/parse-function-declarations.ts (1)
58-67: Exported const name may collide across filesUsing the unqualified functionName for the emitted const can collide when two imports share a name. Until a global naming strategy is wired, at least gate emission on a collision map or surface a diagnostic.
If you want a local, non-invasive guard now, I can add a simple name counter in the printer stage and pass an overridden name into this method.-->
src/parsers/parse-interfaces.ts (1)
42-59: Emitted variable name may clashLike functions, exporting const interfaceName can collide. Consider using a collision-resolved name provided by the printer.
tests/utils.ts (1)
26-37:withTSchemaparameter is effectively unusedSince addImport is false, withTSchema has no effect on output. Either remove the param or drive import shaping through generateCode/createOutputFile consistently.
-export const generateFormattedCode = ( - sourceFile: SourceFile, - withTSchema: boolean = false, -): string => { +export const generateFormattedCode = (sourceFile: SourceFile): string => { const code = generateCode({ sourceCode: sourceFile.getFullText(), callerFile: sourceFile.getFilePath(), project: sourceFile.getProject(), }) - return formatWithPrettier(code, false, withTSchema) + return formatWithPrettier(code, false /* addImport */) }If tests rely on the flag, I can update expectations to use the new entrypoint’s import logic instead.
tests/handlers/typebox/objects.test.ts (1)
44-52: “with export” case correctly produces identical output (always-export policy).
Consider adding a single parametric test to DRY both “with/without export” scenarios since outputs are identical now.src/parsers/parse-type-aliases.ts (1)
8-10: Method indirection OK, but parse() wrapper is redundant.
Either inline intoparseor markparseWithImportFlagprivate to avoid two public entry points doing the same job.src/index.ts (1)
10-37: Import set composition is sound (Type as value, Static/TSchema as type-only).
Minor: consider reusing the caller’s Project to reduce allocations, unless you intentionally want a clean, isolated output file.tests/traverse/dependency-collector.integration.test.ts (2)
11-14: Suite name is outdated.
Rename “DependencyCollector” to “DependencyTraversal” to match the new API.-describe('DependencyCollector', () => { +describe('DependencyTraversal', () => {
35-41: UsestartTraversal’s return value directly
Optional refactor for clarity:- traverser.startTraversal(mainFile) - const dependencies = traverser.getNodesToPrint() + const dependencies = traverser.startTraversal(mainFile)Alias‐import handling is already covered (e.g.
import { User as UserAlias }), so no additional alias tests are needed. Consider adding a test for duplicate type names from different modules to validate collision handling.tests/import-resolution.test.ts (3)
5-5: Rename suite to reflect new architecture (graphology DAG), not ts-morph.Keeps tests aligned with the PR’s intent and avoids confusion.
-describe('ts-morph codegen with imports', () => { +describe('codegen with imports (graphology DAG)', () => {
83-101: Package import resolution: add a variant with import aliasing.Covers
import { ExternalType as ET }and verifies the printer emitsExternalTypeonce and references it consistently.Would you like me to add this test case?
272-289: “with exports” mirrors “without exports”: consider parameterizing to DRY the suite.Same expectations across both groups can be table-driven to reduce duplication.
tests/handlers/typebox/utility-types.test.ts (2)
106-116: Required + Optional interplay: verify semantics in TypeBox.You’re emitting
Type.Required(Type.Object({ a: Type.Optional(...)})). Confirm that Required removes the Optional modifier at property-level in the produced schema; if not, emit the inner properties withoutType.Optionalwhen wrapping withType.Required.I can add an assertion that
JSONSchema7.requiredincludes['a','b']for this case if you expose a small “toSchema” helper in tests.
147-169: Exported KeyOf (with exports): LGTM.Matches the non-exported variant; duplication suggests table-driven tests could reduce noise.
tests/handlers/typebox/interfaces.test.ts (1)
147-161: Empty extension emits Composite with empty object: consider collapsing to alias.Emit
export const Extended = Base(andexport type Extended = Static<typeof Extended>) when there are no additional props to avoid unnecessary composites.Proposed printer tweak (conceptual):
-if (extensionProps.isEmpty()) { - printComposite([Base, Type.Object({})]) -} else { - printComposite([Base, Type.Object(extensionProps)]) -} +print( + extensionProps.isEmpty() + ? id(BaseName) // const Extended = Base + : Type.Composite([id(BaseName), Type.Object(extensionProps)]) +)
📜 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 selected for processing (25)
ARCHITECTURE.md(2 hunks)src/index.ts(1 hunks)src/parsers/parse-enums.ts(1 hunks)src/parsers/parse-function-declarations.ts(2 hunks)src/parsers/parse-interfaces.ts(6 hunks)src/parsers/parse-type-aliases.ts(3 hunks)src/printer/typebox-printer.ts(1 hunks)src/traverse/dependency-traversal.ts(3 hunks)src/traverse/file-graph.ts(1 hunks)src/traverse/node-graph.ts(1 hunks)src/traverse/types.ts(1 hunks)src/utils/generate-qualified-name.ts(1 hunks)tests/handlers/typebox/arrays.test.ts(11 hunks)tests/handlers/typebox/enums.test.ts(1 hunks)tests/handlers/typebox/functions.test.ts(9 hunks)tests/handlers/typebox/interfaces.test.ts(9 hunks)tests/handlers/typebox/objects.test.ts(3 hunks)tests/handlers/typebox/primitive-types.test.ts(8 hunks)tests/handlers/typebox/template-literals.test.ts(1 hunks)tests/handlers/typebox/typeof.test.ts(1 hunks)tests/handlers/typebox/utility-types.test.ts(10 hunks)tests/import-resolution.test.ts(8 hunks)tests/traverse/dependency-collector.integration.test.ts(1 hunks)tests/traverse/dependency-ordering.test.ts(1 hunks)tests/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- src/traverse/file-graph.ts
- src/parsers/parse-enums.ts
- tests/handlers/typebox/typeof.test.ts
- tests/handlers/typebox/template-literals.test.ts
- src/traverse/node-graph.ts
- src/utils/generate-qualified-name.ts
- src/printer/typebox-printer.ts
- tests/handlers/typebox/enums.test.ts
- tests/traverse/dependency-ordering.test.ts
- src/traverse/types.ts
- tests/handlers/typebox/arrays.test.ts
- tests/handlers/typebox/primitive-types.test.ts
🧰 Additional context used
🧬 Code graph analysis (10)
src/index.ts (4)
src/traverse/types.ts (1)
TraversedNode(5-12)src/printer/typebox-printer.ts (1)
TypeBoxPrinter(13-62)src/input-handler.ts (2)
InputOptions(5-10)createSourceFileFromInput(64-101)src/traverse/dependency-traversal.ts (1)
DependencyTraversal(19-279)
tests/import-resolution.test.ts (1)
tests/utils.ts (2)
generateFormattedCode(26-37)formatWithPrettier(16-24)
src/parsers/parse-type-aliases.ts (1)
src/utils/add-static-type-alias.ts (1)
addStaticTypeAlias(4-22)
tests/handlers/typebox/objects.test.ts (1)
tests/utils.ts (3)
generateFormattedCode(26-37)formatWithPrettier(16-24)createSourceFile(12-14)
tests/traverse/dependency-collector.integration.test.ts (3)
src/traverse/types.ts (1)
TraversedNode(5-12)src/traverse/dependency-traversal.ts (1)
DependencyTraversal(19-279)tests/utils.ts (1)
createSourceFile(12-14)
tests/handlers/typebox/functions.test.ts (1)
tests/utils.ts (3)
generateFormattedCode(26-37)formatWithPrettier(16-24)createSourceFile(12-14)
src/traverse/dependency-traversal.ts (4)
src/traverse/file-graph.ts (1)
FileGraph(13-25)src/traverse/node-graph.ts (1)
NodeGraph(8-57)src/traverse/types.ts (1)
TraversedNode(5-12)src/utils/generate-qualified-name.ts (1)
generateQualifiedNodeName(20-25)
tests/utils.ts (1)
src/index.ts (1)
generateCode(53-77)
tests/handlers/typebox/utility-types.test.ts (1)
tests/utils.ts (3)
generateFormattedCode(26-37)formatWithPrettier(16-24)createSourceFile(12-14)
tests/handlers/typebox/interfaces.test.ts (1)
tests/utils.ts (3)
generateFormattedCode(26-37)formatWithPrettier(16-24)createSourceFile(12-14)
🪛 LanguageTool
ARCHITECTURE.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...cture ## Table of Contents - Overview - Core Components - [...
(QB_NEW_EN)
[grammar] ~6-~6: There might be a mistake here.
Context: ...Overview - Core Components - [Code Generation Flow](#code-generation-f...
(QB_NEW_EN)
[grammar] ~7-~7: There might be a mistake here.
Context: ...re-components) - Code Generation Flow - TypeBox Printer - [...
(QB_NEW_EN)
[grammar] ~8-~8: There might be a mistake here.
Context: ...de-generation-flow) - TypeBox Printer - [Dependency Management](#dependency-manag...
(QB_NEW_EN)
[grammar] ~9-~9: There might be a mistake here.
Context: ...ebox-printer) - Dependency Management - Parser System - [Hand...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...ependency-management) - Parser System - Handler System - [Im...
(QB_NEW_EN)
[grammar] ~11-~11: There might be a mistake here.
Context: ...tem](#parser-system) - Handler System - Import Resolution ...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...(#handler-system) - Import Resolution - Input Handling - [In...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...(#import-resolution) - Input Handling - [Interface Inheritance](#interface-inheri...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...put-handling) - Interface Inheritance - Utility Functions -...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...face-inheritance) - Utility Functions - Process Overview - [...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...(#utility-functions) - Process Overview - Basic Usage - [Testing](#...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...rview](#process-overview) - Basic Usage - Testing ## Overview `@daxse...
(QB_NEW_EN)
[grammar] ~26-~26: There might be a mistake here.
Context: ...rfaces, enums, and function declarations - Generic Types: Generic interfaces and ...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ...rameters with proper constraint handling - Complex Types: Union and intersection ...
(QB_NEW_EN)
[grammar] ~28-~28: There might be a mistake here.
Context: ...bject structures, template literal types - Utility Types: Built-in support for Pi...
(QB_NEW_EN)
[grammar] ~29-~29: There might be a mistake here.
Context: ...cord, and other TypeScript utility types - Advanced Features: Conditional types, ...
(QB_NEW_EN)
[grammar] ~30-~30: There might be a mistake here.
Context: ...s, keyof operators, indexed access types - Import Resolution: Cross-file type dep...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...Creates a SourceFile from input using createSourceFileFromInput 2. Generic Interface Detection: Checks fo...
(QB_NEW_EN)
[grammar] ~43-~43: There might be a mistake here.
Context: ...ssary @sinclair/typebox imports using createOutputFile 4. Dependency Traversal: Uses `Dependency...
(QB_NEW_EN)
[grammar] ~45-~45: There might be a mistake here.
Context: ... sorted nodes using TypeBoxPrinter in printSortedNodes 6. Output Formatting: Returns the formatt...
(QB_NEW_EN)
[grammar] ~65-~65: There might be a mistake here.
Context: ...versal.ts"> - Main orchestrator 2. FileGraph: <mcfile name="file-graph.ts...
(QB_NEW_EN)
[grammar] ~66-~66: There might be a mistake here.
Context: ...mcfile> - Tracks file-level dependencies 3. NodeGraph: <mcfile name="node-graph.ts...
(QB_NEW_EN)
[grammar] ~71-~71: There might be a mistake here.
Context: ...Adds all types from the main source file 2. Import Processing: Recursively process...
(QB_NEW_EN)
[grammar] ~72-~72: There might be a mistake here.
Context: ...ecursively processes import declarations 3. Dependency Extraction: Analyzes type r...
(QB_NEW_EN)
[grammar] ~73-~73: There might be a mistake here.
Context: ...ype references to build dependency graph 4. Topological Sorting: Returns nodes in ...
(QB_NEW_EN)
[grammar] ~84-~84: There might be a mistake here.
Context: ...des: - Common interface for all parsers - Shared access to output SourceFile, Ty...
(QB_NEW_EN)
[grammar] ~85-~85: There might be a mistake here.
Context: ...pt printer, and processed types tracking - Abstract parse method for implementati...
(QB_NEW_EN)
[grammar] ~90-~90: There might be a mistake here.
Context: ...dles both regular and generic interfaces 2. TypeAliasParser: <mcfile name="parse-t...
(QB_NEW_EN)
[grammar] ~91-~91: There might be a mistake here.
Context: ...ile> - Processes type alias declarations 3. EnumParser: <mcfile name="parse-enums....
(QB_NEW_EN)
[grammar] ~92-~92: There might be a mistake here.
Context: ...s"> - Handles enum declarations 4. FunctionParser: <mcfile name="parse-fu...
(QB_NEW_EN)
[grammar] ~93-~93: There might be a mistake here.
Context: ...cfile> - Processes function declarations 5. ImportParser: <mcfile name="parse-impo...
(QB_NEW_EN)
[grammar] ~123-~123: There might be a mistake here.
Context: ...pports nested import chains of any depth - Circular Import Detection: Prevents in...
(QB_NEW_EN)
[grammar] ~124-~124: There might be a mistake here.
Context: ...Prevents infinite loops during traversal - Qualified Naming: Resolves type name c...
(QB_NEW_EN)
[grammar] ~146-~146: There might be a mistake here.
Context: ...either file paths or source code strings 2. Path Resolution: Handles both absolute...
(QB_NEW_EN)
[grammar] ~147-~147: There might be a mistake here.
Context: ...lative file paths with proper validation 3. Relative Import Validation: Prevents r...
(QB_NEW_EN)
[grammar] ~148-~148: There might be a mistake here.
Context: ...nless a callerFile context is provided 4. Project Context Sharing: Supports pass...
(QB_NEW_EN)
[grammar] ~149-~149: There might be a mistake here.
Context: ...es to maintain import resolution context 5. Error Handling: Provides clear error m...
(QB_NEW_EN)
[grammar] ~192-~192: There might be a mistake here.
Context: ...resenting the equivalent TypeBox schema. - : Generates and adds the `export type [T...
(QB_NEW_EN)
[grammar] ~217-~217: There might be a mistake here.
Context: ...ts local types from the main source file - Recursively traverses import chains to g...
(QB_NEW_EN)
[grammar] ~218-~218: There might be a mistake here.
Context: ...import chains to gather all dependencies - Establishes dependency relationships bet...
(QB_NEW_EN)
[grammar] ~219-~219: There might be a mistake here.
Context: ...pendency relationships between all types - Returns topologically sorted nodes for p...
(QB_NEW_EN)
[grammar] ~248-~248: There might be a mistake here.
Context: ...onent testing using Bun's test framework - Integration Tests: End-to-end testing ...
(QB_NEW_EN)
[grammar] ~249-~249: There might be a mistake here.
Context: ...ting of the complete generation pipeline - Type Safety Tests: Validation of gener...
(QB_NEW_EN)
[grammar] ~250-~250: There might be a mistake here.
Context: ... Validation of generated TypeBox schemas - Edge Case Coverage: Testing of complex...
(QB_NEW_EN)
[grammar] ~255-~255: There might be a mistake here.
Context: ...peScript Compiler**: Type checking with bun tsc - ESLint: Code quality and style enforce...
(QB_NEW_EN)
[grammar] ~256-~256: There might be a mistake here.
Context: ...nt**: Code quality and style enforcement - Prettier: Consistent code formatting -...
(QB_NEW_EN)
[grammar] ~257-~257: There might be a mistake here.
Context: ...Prettier: Consistent code formatting - Bun Test: Fast and reliable test execu...
(QB_NEW_EN)
🔇 Additional comments (36)
tests/handlers/typebox/objects.test.ts (3)
16-24: Tests updated to expect exported runtime and static type. LGTM.
30-36: Tuple expectations reflect exported artifacts. LGTM.
58-64: Tuple with export aligned with new generator behavior.tests/handlers/typebox/functions.test.ts (11)
29-35: Parameterized function type — expected exports correct.
41-47: Optional parameter mapping to Type.Optional is correct.
55-61: Exported input still yields exported artifacts — consistent.
70-76: Params with export — OK.
85-91: Optional param with export — OK.
101-107: Function declaration → Type.Function mapping is correct.
116-122: Params preserved in order and kind — OK.
128-134: Void return handled via Type.Void — OK.
142-148: Exported decl — consistent with always-export policy.
157-163: Exported params variant — OK.
172-178: Exported optional params — OK.src/index.ts (1)
39-51: Printer loop over topologically sorted nodes — clear and correct.tests/import-resolution.test.ts (2)
38-54: Export-everything expectation: LGTM.Topological order places ExternalType before User; aliasing via Static is consistent.
139-161: Three-level chain: also test independence from file creation order.Randomize file creation order (shuffle createSourceFile calls) to ensure graph-based topo sort, not authoring order, drives emission.
tests/handlers/typebox/utility-types.test.ts (12)
12-34: Exported KeyOf output: LGTM.Shape and Static alias are correct.
41-46: Record: LGTM.Covers string→number mapping correctly.
53-63: Partial: LGTM.Object wrapper and Static alias are consistent with policy.
70-81: Pick: LGTM.Literal key selection via Type.Literal("a") is correct.
88-99: Omit: LGTM.Matches expected transformation.
131-143: Indexed Access: LGTM.
Type.Index(A, Type.Literal("a"))is the right emission and exports are consistent.
176-181: Exported Record: LGTM.
188-199: Exported Partial: LGTM.
205-216: Exported Pick: LGTM.
223-234: Exported Omit: LGTM.
241-251: Exported Required: apply the same Required-vs-Optional check here.Mirror the verification suggested above.
267-279: Exported Indexed Access: LGTM.tests/handlers/typebox/interfaces.test.ts (7)
16-23: Interface → exported Type.Object: LGTM.Emission and Static aliasing are consistent.
52-68: Single inheritance via Composite: LGTM.
Type.Composite([Base, Type.Object({...})])is correct and ordered.
82-105: Multiple inheritance ordering: LGTM.A before B before extension props preserves property precedence.
175-201: Nested inheritance chain: LGTM.Composite layering A→B→C reads correctly.
214-233: Generics with concrete instantiation: LGTM.Factory form
<T extends TSchema>(T: T)andB = Composite([A(Number), ...])is coherent.
244-262: Generic extension factory: LGTM.
B<T>(T)delegating toA(T)preserves variance; import ofTSchemais handled via the test harness flag.
277-297: Literal-union generic constraint: LGTM.Dual namespace reuse (
const Aandtype A) is valid in TS and keeps names stable across exports.
| export const generateCode = ({ sourceCode, filePath, ...options }: InputOptions): string => { | ||
| // Create source file from input | ||
| const sourceFile = createSourceFileFromInput({ | ||
| sourceCode, | ||
| filePath, | ||
| ...options, | ||
| }) | ||
|
|
||
| // Create dependency traversal and start traversal | ||
| const dependencyTraversal = new DependencyTraversal() | ||
| const traversedNodes = dependencyTraversal.startTraversal(sourceFile) | ||
|
|
||
| // Check if any interfaces have generic type parameters | ||
| const hasGenericInterfaces = traversedNodes.some( | ||
| (t) => Node.isInterfaceDeclaration(t.node) && t.node.getTypeParameters().length > 0, | ||
| ) | ||
|
|
||
| // Create output file with proper imports | ||
| const newSourceFile = createOutputFile(hasGenericInterfaces) | ||
|
|
||
| // Print sorted nodes to output | ||
| const result = printSortedNodes(traversedNodes, newSourceFile) | ||
|
|
||
| return result | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proactive collision detection before printing.
Given parsers emit original names, detect duplicate originalNames across different qualifiedNames and fail fast with guidance (see proposed diff in parse-type-aliases.ts comment). This prevents invalid, duplicate symbol emissions.
🤖 Prompt for AI Agents
In src/index.ts around lines 53 to 77, add a proactive collision detection step
after traversing nodes and before creating the output file/printing: scan
traversedNodes to group by originalName and collect their distinct qualifiedName
values, and if any originalName is associated with more than one qualifiedName
throw a clear, failing error that explains which originalName collides, lists
the differing qualifiedNames and advises renaming or scoping to avoid duplicate
emissions; implement this check synchronously and return/throw before calling
createOutputFile or printSortedNodes so the build fails fast with actionable
guidance.
| if (!functionName) return | ||
|
|
||
| if (this.processedTypes.has(functionName)) { | ||
| return | ||
| } | ||
| if (this.processedTypes.has(functionName)) return | ||
| this.processedTypes.add(functionName) |
There was a problem hiding this comment.
Dedupe key uses bare name; risks skipping same-named functions from different files
processedTypes keyed by functionName causes false “already processed” when two files export the same name. Plumb a qualified key (e.g., file-hashed) or accept a provided output name from traversal/printing to avoid collisions and incorrect skips.
I recommend updating BaseParser/TypeBoxPrinter to pass a collision-safe outputName (qualified) into parse(), and track processedTypes by that key. Would you like a follow-up patch touching BaseParser + all parsers?
| // Add generic type alias: type A<T extends TSchema> = Static<ReturnType<typeof A<T>>> | ||
| this.addGenericTypeAlias(interfaceName, typeParameters, isExported) | ||
| this.addGenericTypeAlias(interfaceName, typeParameters) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potentially invalid TypeScript: typeof with type arguments
Creating a TypeQuery with type arguments (typeof A<T>) is not valid in TypeScript; many TS versions reject this. This risks broken emitted code for generics.
Please run tsc to confirm. If invalid, switch to a safe pattern like:
- Emit
const A = <T extends TSchema>(...) => ...andtype A<T extends TSchema> = Static<ReturnType<ReturnType<typeof A>>>, or - Emit a named helper type and map
Tseparately withouttypeoftype args.
I can produce a concrete patch once we confirm the preferred generic emission shape.
Also applies to: 89-145
🏁 Script executed:
#!/bin/bash
fd parse-interfaces.ts -t fLength of output: 61
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' src/parsers/parse-interfaces.tsLength of output: 4568
Replace invalid typeof A<T> usage
TypeScript type queries don’t support type arguments (queries accept only plain identifiers) (1library.net); emit a generic helper function (e.g. const AHelper = <T extends TSchema>(…) => …) and then alias type A<T extends TSchema> = Static<ReturnType<typeof AHelper<T>>>.
🤖 Prompt for AI Agents
In src/parsers/parse-interfaces.ts around lines 85 to 87, the code emits an
invalid type query using typeof A<T> which TypeScript does not allow; instead
create a generic helper function/const (e.g. const AHelper = <T extends
TSchema>(...) => /* returns schema builder */) and then emit the alias using the
helper: type A<T extends TSchema> = Static<ReturnType<typeof AHelper>> but with
the generic applied via the helper—i.e. generate the helper declaration with the
same generic params and change addGenericTypeAlias to reference
ReturnType<typeof AHelper> while keeping the generic bounds on the alias; ensure
the helper name is unique and exported if needed so the alias compiles.
| parseWithImportFlag(typeAlias: TypeAliasDeclaration): void { | ||
| const typeName = typeAlias.getName() | ||
|
|
||
| if (this.processedTypes.has(typeName)) { | ||
| return | ||
| } | ||
|
|
||
| this.processedTypes.add(typeName) | ||
|
|
||
| const typeNode = typeAlias.getTypeNode() |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Removed processedTypes dedup guard — verify no duplicate emissions.
Since TypeBoxPrinter still wires a processedTypes set, confirm no other parser relies on it and that graph traversal guarantees uniqueness. Otherwise, identical nodes may be printed twice if ever requeued.
Run quick checks:
🏁 Script executed:
#!/bin/bash
# Where else is processedTypes used?
rg -n "processedTypes" -C2
# Ensure TypeAliasParser no longer uses it
rg -n "class TypeAliasParser" -n -C5 src | sed -n '1,160p'Length of output: 4112
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' src/printer/typebox-printer.tsLength of output: 2165
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' src/parsers/parse-type-aliases.tsLength of output: 1379
Add processedTypes guard in TypeAliasParser
In src/parsers/parse-type-aliases.ts, parseWithImportFlag no longer checks or adds to this.processedTypes, so the same alias could be emitted twice. At the top of parseWithImportFlag, add:
if (this.processedTypes.has(typeName)) return;
this.processedTypes.add(typeName);🤖 Prompt for AI Agents
In src/parsers/parse-type-aliases.ts around lines 12 to 15, parseWithImportFlag
does not check or mark processed types so the same alias can be emitted twice;
add a guard at the top of parseWithImportFlag that returns immediately if
this.processedTypes.has(typeName) and otherwise adds the type name with
this.processedTypes.add(typeName) before proceeding.
| extractDependencies(): void { | ||
| // Extract dependencies for all nodes in the graph | ||
| for (const nodeId of this.nodeGraph.nodes()) { | ||
| const nodeData = this.nodeGraph.getNode(nodeId) | ||
|
|
||
| // Remove unused imported types | ||
| const nodesToRemove: string[] = [] | ||
| this.dependencyGraph.forEachNode((nodeName, nodeData) => { | ||
| if (nodeData.type === 'typeAlias' && nodeData.isImported && !usedTypes.has(nodeName)) { | ||
| nodesToRemove.push(nodeName) | ||
| } | ||
| }) | ||
| for (const nodeName of nodesToRemove) { | ||
| this.dependencyGraph.dropNode(nodeName) | ||
| } | ||
| } | ||
| if (nodeData.type === 'typeAlias') { | ||
| const typeAlias = nodeData.node as TypeAliasDeclaration | ||
| const typeNode = typeAlias.getTypeNode() | ||
| if (!typeNode) continue | ||
|
|
||
| /** | ||
| * Get topologically sorted types with preference for imported types first | ||
| */ | ||
| getTopologicallySortedTypes(exportEverything: boolean): TypeDependency[] { | ||
| const typeNodes = this.dependencyGraph.filterNodes( | ||
| (_, attributes) => attributes.type === 'typeAlias', | ||
| ) | ||
| if (typeNodes.length === 0) return [] | ||
|
|
||
| for (const typeName of typeNodes) { | ||
| const nodeData = this.dependencyGraph.getNodeAttributes(typeName) | ||
| if (!nodeData || nodeData.type !== 'typeAlias') continue | ||
|
|
||
| const typeNode = nodeData.declaration.getTypeNode() | ||
| if (typeNode) { | ||
| const typeReferences = this.extractTypeReferences(typeNode) | ||
| for (const ref of typeReferences) { | ||
| if (this.dependencyGraph.hasNode(ref) && !this.dependencyGraph.hasEdge(ref, typeName)) { | ||
| this.dependencyGraph.addEdge(ref, typeName, { | ||
| type: 'REFERENCES', | ||
| direct: true, | ||
| context: 'type-dependency', | ||
| }) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const sortedNodes = topologicalSort(this.dependencyGraph) | ||
| const sortedDependencies = sortedNodes | ||
| .map((nodeId: string) => { | ||
| const nodeData = this.dependencyGraph.getNodeAttributes(nodeId) | ||
| if (nodeData && nodeData.type === 'typeAlias') { | ||
| return { | ||
| typeAlias: nodeData.declaration, | ||
| sourceFile: nodeData.sourceFile, | ||
| isImported: nodeData.isImported, | ||
| // Add edges for dependencies | ||
| for (const referencedType of typeReferences) { | ||
| if (this.nodeGraph.hasNode(referencedType)) { | ||
| this.nodeGraph.addDependency(referencedType, nodeId) | ||
| } | ||
| } | ||
| return undefined | ||
| }) | ||
| .filter((dep): dep is TypeDependency => dep !== undefined) | ||
|
|
||
| if (!exportEverything) { | ||
| // Filter out unused imports when not exporting everything | ||
| this.filterUnusedImports() | ||
| // Re-get the sorted types after filtering by rebuilding the graph | ||
| const filteredSortedNodes = topologicalSort(this.dependencyGraph) | ||
| const filteredDependencies = filteredSortedNodes | ||
| .map((nodeId: string) => { | ||
| const nodeData = this.dependencyGraph.getNodeAttributes(nodeId) | ||
| if (nodeData && nodeData.type === 'typeAlias') { | ||
| return { | ||
| typeAlias: nodeData.declaration, | ||
| sourceFile: nodeData.sourceFile, | ||
| isImported: nodeData.isImported, | ||
| } | ||
| } | ||
| return undefined | ||
| }) | ||
| .filter((dep): dep is TypeDependency => dep !== undefined) | ||
|
|
||
| // For exportEverything=false, still prioritize imported types while respecting dependencies | ||
| const processed = new Set<string>() | ||
| const result: TypeDependency[] = [] | ||
| const remaining = [...filteredDependencies] | ||
|
|
||
| while (remaining.length > 0) { | ||
| // Find all types with satisfied dependencies | ||
| const readyTypes = remaining.filter((dep) => this.allDependenciesProcessed(dep, processed)) | ||
|
|
||
| if (readyTypes.length === 0) { | ||
| // If no types are ready, take the first one to avoid infinite loop | ||
| const typeToAdd = remaining.shift() | ||
| if (typeToAdd) { | ||
| result.push(typeToAdd) | ||
| processed.add(typeToAdd.typeAlias.getName()) | ||
| } else if (nodeData.type === 'interface') { | ||
| const interfaceDecl = nodeData.node as InterfaceDeclaration | ||
| const typeReferences = this.extractTypeReferences(interfaceDecl) | ||
|
|
||
| // Add edges for dependencies | ||
| for (const referencedType of typeReferences) { | ||
| if (this.nodeGraph.hasNode(referencedType)) { | ||
| this.nodeGraph.addDependency(referencedType, nodeId) | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| // Among ready types, prefer imported types first, then types with dependencies | ||
| const importedReady = readyTypes.filter((dep) => dep.isImported) | ||
| let typeToAdd: TypeDependency | ||
|
|
||
| if (importedReady.length > 0) { | ||
| typeToAdd = importedReady[0]! | ||
| } else { | ||
| // Among non-imported ready types, prefer those that have dependencies | ||
| const withDependencies = readyTypes.filter((dep) => { | ||
| const typeNode = dep.typeAlias.getTypeNode() | ||
| if (!typeNode) return false | ||
| const refs = this.extractTypeReferences(typeNode) | ||
| return refs.length > 0 | ||
| }) | ||
| typeToAdd = withDependencies.length > 0 ? withDependencies[0]! : readyTypes[0]! | ||
| } | ||
|
|
||
| result.push(typeToAdd) | ||
| processed.add(typeToAdd.typeAlias.getName()) | ||
|
|
||
| // Remove the processed type from remaining | ||
| const index = remaining.indexOf(typeToAdd) | ||
| if (index > -1) { | ||
| remaining.splice(index, 1) | ||
| } | ||
| } | ||
|
|
||
| return result | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Dependencies not extracted for functions or interface heritage
- Function nodes are ignored, so parameter/return type deps aren’t ordered.
- Extends clauses use ExpressionWithTypeArguments, not TypeReference; those base types are missed.
} else if (nodeData.type === 'interface') {
- const interfaceDecl = nodeData.node as InterfaceDeclaration
- const typeReferences = this.extractTypeReferences(interfaceDecl)
+ const interfaceDecl = nodeData.node as InterfaceDeclaration
+ const typeReferences = this.extractTypeReferences(interfaceDecl)
// Add edges...
for (const referencedType of typeReferences) {
if (this.nodeGraph.hasNode(referencedType)) {
this.nodeGraph.addDependency(referencedType, nodeId)
}
}
+ } else if (nodeData.type === 'function') {
+ const fn = nodeData.node
+ const typeReferences = this.extractTypeReferences(fn)
+ for (const referencedType of typeReferences) {
+ if (this.nodeGraph.hasNode(referencedType)) {
+ this.nodeGraph.addDependency(referencedType, nodeId)
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/traverse/dependency-traversal.ts around lines 115–145,
extractDependencies currently only handles typeAlias and interface bodies, which
misses dependencies from function parameter/return types and interface heritage
(extends) clauses. Update the function to: 1) handle function nodes (e.g.,
FunctionDeclaration/FunctionExpression/MethodSignature/Signature nodes) by
extracting type references from each parameter type and the return type (use the
existing extractTypeReferences on the parameter/return type nodes or signatures)
and add dependency edges when referenced types exist in nodeGraph; 2) for
interfaces, also walk heritage/extends clauses (ExpressionWithTypeArguments) to
extract the referenced base type identifiers (or underlying type nodes) and add
dependency edges after verifying nodeGraph.hasNode; keep the same pattern of
checking hasNode before calling addDependency and reuse extractTypeReferences
where applicable.
| collectFromImports(importDeclarations: ImportDeclaration[], isMainCode: boolean): void { | ||
| for (const importDecl of importDeclarations) { | ||
| const moduleSourceFile = importDecl.getModuleSpecifierSourceFile() | ||
| if (!moduleSourceFile) continue | ||
|
|
||
| if (readyTypes.length > 0) { | ||
| // Among ready types, prefer imported types first | ||
| const importedReady = readyTypes.filter((item) => item.dep.isImported) | ||
|
|
||
| let typeToAdd: { index: number; dep: TypeDependency } | undefined | ||
|
|
||
| if (importedReady.length > 0) { | ||
| // If there are imported types ready, pick the first one | ||
| typeToAdd = importedReady[0] | ||
| } else { | ||
| // Among local types, prefer those that have dependencies on already processed types | ||
| // This ensures types that depend on imported types come right after their dependencies | ||
| const localReady = readyTypes.filter((item) => !item.dep.isImported) | ||
| const withProcessedDeps = localReady.filter((item) => { | ||
| const typeNode = item.dep.typeAlias.getTypeNode() | ||
| if (!typeNode) return false | ||
| const refs = this.extractTypeReferences(typeNode) | ||
| return refs.some((ref) => processed.has(ref)) | ||
| }) | ||
|
|
||
| typeToAdd = withProcessedDeps.length > 0 ? withProcessedDeps[0] : localReady[0] | ||
| } | ||
| const filePath = moduleSourceFile.getFilePath() | ||
|
|
||
| if (typeToAdd) { | ||
| result.push(typeToAdd.dep) | ||
| processed.add(typeToAdd.dep.typeAlias.getName()) | ||
| remaining.splice(typeToAdd.index, 1) | ||
| addedInThisRound = true | ||
| } | ||
| } | ||
| // Prevent infinite loops by tracking visited files | ||
| if (this.fileGraph.hasNode(filePath)) continue | ||
|
|
||
| // Safety check to prevent infinite loop | ||
| if (!addedInThisRound) { | ||
| // Add the first remaining item to break the loop | ||
| const dep = remaining.shift()! | ||
| result.push(dep) | ||
| processed.add(dep.typeAlias.getName()) | ||
| } | ||
| } | ||
| this.fileGraph.addFile(filePath, moduleSourceFile) | ||
|
|
||
| return result | ||
| } | ||
| const imports = moduleSourceFile.getImportDeclarations() | ||
| const typeAliases = moduleSourceFile.getTypeAliases() | ||
| const interfaces = moduleSourceFile.getInterfaces() | ||
| const enums = moduleSourceFile.getEnums() | ||
| const functions = moduleSourceFile.getFunctions() | ||
|
|
||
| /** | ||
| * Check if all dependencies of a type have been processed | ||
| */ | ||
| private allDependenciesProcessed(dependency: TypeDependency, processed: Set<string>): boolean { | ||
| const typeNode = dependency.typeAlias.getTypeNode() | ||
| if (!typeNode) return true | ||
|
|
||
| const references = this.extractTypeReferences(typeNode) | ||
| for (const ref of references) { | ||
| if (!processed.has(ref)) { | ||
| return false | ||
| // Add all imported types to the graph |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Import traversal adds every declaration from a file (noise, collisions, slow)
You’re adding all declarations from each imported module, even if not imported. This bloats the node graph and, combined with “never prune direct imports,” forces unnecessary emission.
Filter by actual imported specifiers (and only exported declarations). Minimal improvement:
- const imports = moduleSourceFile.getImportDeclarations()
- const typeAliases = moduleSourceFile.getTypeAliases()
- const interfaces = moduleSourceFile.getInterfaces()
- const enums = moduleSourceFile.getEnums()
- const functions = moduleSourceFile.getFunctions()
+ const imports = moduleSourceFile.getImportDeclarations()
+ const named = new Set(importDecl.getNamedImports().map((ni) => ni.getName()))
+ const typeAliases = moduleSourceFile.getTypeAliases().filter((d) => named.has(d.getName()))
+ const interfaces = moduleSourceFile.getInterfaces().filter((d) => named.has(d.getName()))
+ const enums = moduleSourceFile.getEnums().filter((d) => named.has(d.getName()))
+ const functions = moduleSourceFile.getFunctions().filter((d) => named.has(d.getName()))Follow-up: handle default and namespace imports (map to default export symbol and selectively include only referenced members). I can extend this if desired.
Also applies to: 169-180, 182-196, 198-209, 211-224
🤖 Prompt for AI Agents
In src/traverse/dependency-traversal.ts around lines 150 to 168 (and similarly
for ranges 169-180, 182-196, 198-209, 211-224), the code unconditionally adds
every declaration from an imported module; change this so you only add
declarations that correspond to actual import specifiers and are exported from
the module: parse the ImportDeclaration to collect named specifiers, resolve
each specifier to the module's exported symbol (handle named imports, map
default imports to the module's default export, and treat namespace imports by
selecting only referenced members), then filter the module's declarations to
include only those exported symbols referenced by the import specifiers before
adding nodes to the fileGraph; ensure you still avoid infinite loops with the
existing fileGraph checks and perform this selective filtering at each of the
other listed line ranges.
| expect(generateFormattedCode(userFile)).toBe( | ||
| formatWithPrettier(` | ||
| const VeryDeeplyNestedType = Type.Object({ | ||
| export const VeryDeeplyNestedType = Type.Object({ | ||
| core: Type.String(), | ||
| }) | ||
| type VeryDeeplyNestedType = Static<typeof VeryDeeplyNestedType> | ||
| export type VeryDeeplyNestedType = Static<typeof VeryDeeplyNestedType> | ||
| const DeeplyNestedType = Type.Object({ | ||
| export const DeeplyNestedType = Type.Object({ | ||
| value: Type.Boolean(), | ||
| veryDeep: VeryDeeplyNestedType, | ||
| }) | ||
| type DeeplyNestedType = Static<typeof DeeplyNestedType> | ||
| export type DeeplyNestedType = Static<typeof DeeplyNestedType> | ||
| const IntermediateType = Type.Object({ | ||
| export const IntermediateType = Type.Object({ | ||
| id: Type.String(), | ||
| nested: DeeplyNestedType, | ||
| }) | ||
| type IntermediateType = Static<typeof IntermediateType> | ||
| export type IntermediateType = Static<typeof IntermediateType> | ||
| const UltimateUser = Type.Object({ | ||
| export const UltimateUser = Type.Object({ | ||
| name: Type.String(), | ||
| data: IntermediateType, | ||
| }) | ||
| type UltimateUser = Static<typeof UltimateUser> | ||
| export type UltimateUser = Static<typeof UltimateUser> | ||
| `), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Four-level chain: add a cycle-detection test.
Given the DAG traversal, we should assert a clear, actionable error for cycles.
Proposed new test (add near this block):
test('detects and reports import cycles', () => {
createSourceFile(project, `import { B } from './b'; export type A = B;`, 'a.ts')
createSourceFile(project, `import { C } from './c'; export type B = C;`, 'b.ts')
const userFile = createSourceFile(project, `import { A } from './a'; export type C = A;`, 'c.ts')
expect(() => generateFormattedCode(userFile)).toThrow(/cycle|acyclic|DAG/i)
})🤖 Prompt for AI Agents
In tests/import-resolution.test.ts around lines 213 to 242, add a new test that
injects a simple 3-file import cycle (a.ts -> b.ts -> c.ts -> a.ts) and asserts
that generateFormattedCode throws a clear error for cycles; create the three
source files with createSourceFile(project, source, filename) as shown in the
proposal, then wrap generateFormattedCode(userFile) in expect(() =>
...).toThrow(/cycle|acyclic|DAG/i) so the test fails when an import-cycle error
is not raised or not reported with an actionable message.
No description provided.