feat: Add resolver store for type name management #21
Conversation
This change introduces a `ResolverStore` class to manage the mapping between original type names, qualified type names, and type aliases. The key features of this change are: - Generates a unique qualified name for each type based on the original name and the source file path to prevent naming conflicts. - Maintains indexes for looking up type mappings by original name, qualified name, and alias name. - Provides methods to add, retrieve, and resolve type mappings. - Caches the generated qualified names to improve performance. - Clears the resolver store in the test setup to ensure a clean state for each test. The `resolverStore` is used in the `DependencyTraversal` tests to ensure that types are properly resolved and ordered based on their dependencies.
📝 WalkthroughSummary by CodeRabbit
WalkthroughIntroduces a ResolverStore for qualified-name and alias resolution and integrates it across traversal, import/local collection, type-reference extraction, and graph visualization. Propagates aliasName through traversed nodes into parsers and the TypeBox printer. Updates type-reference handling to resolve aliases. Removes generate-qualified-name utility. Adds/updates tests to validate alias-based disambiguation and reset resolver state. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Traversal as DependencyTraversal
participant Importer as ImportCollector
participant Local as LocalTypeCollector
participant Resolver as ResolverStore
participant Graph as NodeGraph
participant Extractor as TypeReferenceExtractor
participant Printer as TypeBoxPrinter
participant Parsers as Parsers (Alias-aware)
Dev->>Traversal: startTraversal(sourceFile)
Traversal->>Importer: getImportDeclarations(sourceFile)
Importer->>Resolver: addTypeMapping(originalName, sourceFile, aliasName?)
Importer->>Graph: addTypeNode({...qualifiedName, aliasName?})
Traversal->>Local: addLocalTypes(sourceFile)
Local->>Resolver: addTypeMapping(originalName, sourceFile)
Local->>Graph: addTypeNode({...qualifiedName})
Traversal->>Extractor: extractDependencies(nodes)
Extractor->>Resolver: resolveQualifiedName(typeName or alias)
Extractor-->>Traversal: requiredNodeIds
Traversal->>Resolver: getAllQualifiedNames()
Traversal->>Graph: topologicalSort(filtered required)
Traversal-->>Printer: TraversedNode[]{ aliasName?, qualifiedName }
Printer->>Parsers: parse(node, aliasName?)
Parsers->>Resolver: resolveAliasName(originalName)
Parsers-->>Printer: Generated TypeBox AST
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
✨ 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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
tests/traverse/dependency-traversal.integration.test.ts (1)
220-233: Add missing alias declaration forUserAlias
The expected output in tests/traverse/dependency-traversal.integration.test.ts (around lines 220–233) referencesUserAliaswithout ever declaring it. You must emit an alias binding before using it. For example:export type User = Static<typeof User>; - + + export const UserAlias = User; + + export type UserAlias = Static<typeof UserAlias>; + export const LocalType = Type.Object({ user: User, userAlias: UserAlias, });src/utils/graph-visualizer.ts (2)
53-57: Bun usage will crash under Node; also duplicate writes—prefer fs onlyRemove Bun writes, ensure directory exists first, then write once.
- const file = Bun.file(outputPath) - await file.write(htmlContent) - - mkdirSync(dirname(outputPath), { recursive: true }) - writeFileSync(outputPath, htmlContent, 'utf8') + mkdirSync(dirname(outputPath), { recursive: true }) + writeFileSync(outputPath, htmlContent, 'utf8')
325-331: Fix graphology UMD usage—current code references Graph.DirectedGraph off a constructorUse the namespace to access DirectedGraph.
- // 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(); + // Use global variables from UMD builds + const Graphology = window.graphology; + const Sigma = window.Sigma; + + // Create a new graph instance + const graph = new Graphology.DirectedGraph();src/traverse/import-collector.ts (1)
17-22: Alias mappings lost when a file was already visited—build alias map before visited-check and persist aliasesWhen the same module is imported multiple times with different aliases, the early
continueskips recording new aliases. Build the alias map first and, if visited, persist alias-only mappings.const filePath = moduleSourceFile.getFilePath() - // Prevent infinite loops by tracking visited files - if (this.fileGraph.hasNode(filePath)) continue - this.fileGraph.addFile(filePath, moduleSourceFile) - - // Build alias map specific to this import declaration - const aliasMap = new Map<string, string>() // originalName -> aliasName - const namedImports = importDecl.getNamedImports() - for (const namedImport of namedImports) { - const originalName = namedImport.getName() - const aliasName = namedImport.getAliasNode()?.getText() - if (aliasName) { - aliasMap.set(originalName, aliasName) - } - } + // Build alias map specific to this import declaration (before visited check) + const aliasMap = new Map<string, string>() // originalName -> aliasName + for (const namedImport of importDecl.getNamedImports()) { + const originalName = namedImport.getName() + const aliasName = namedImport.getAliasNode()?.getText() + if (aliasName) aliasMap.set(originalName, aliasName) + } + + // Prevent infinite loops by tracking visited files + if (this.fileGraph.hasNode(filePath)) { + // Still record alias mappings for this import declaration + for (const [originalName, aliasName] of aliasMap) { + resolverStore.addTypeMapping({ + originalName, + sourceFile: moduleSourceFile, + aliasName, + }) + } + continue + } + this.fileGraph.addFile(filePath, moduleSourceFile)Also applies to: 23-33
src/parsers/parse-interfaces.ts (1)
8-13: Dedup should key off qualified identity, not alias stringUsing
interfaceName(possibly an import alias) as theprocessedTypeskey risks collisions or unintended dedup if two different declarations share the same alias, and also prevents dedup if the same declaration is referenced via two aliases. Prefer a stable, declaration-scoped key (qualified name or filePath::originalName) while still emitting the alias as the exported symbol.Apply:
@@ -export class InterfaceParser extends BaseParser { - parse(interfaceDecl: InterfaceDeclaration, aliasName?: string): void { - const interfaceName = aliasName || interfaceDecl.getName() +import { resolverStore } from '@daxserver/validation-schema-codegen/utils/resolver-store' + +export class InterfaceParser extends BaseParser { + parse(interfaceDecl: InterfaceDeclaration, aliasName?: string): void { + const interfaceName = aliasName || interfaceDecl.getName() @@ - if (this.processedTypes.has(interfaceName)) return - this.processedTypes.add(interfaceName) + // Dedup by qualified identity; still emit "interfaceName" as the symbol + const qKey = + resolverStore.resolveQualifiedName(interfaceName) ?? + `${this.newSourceFile.getFilePath()}::${interfaceDecl.getName()}` + if (this.processedTypes.has(qKey)) return + this.processedTypes.add(qKey)src/traverse/type-reference-extractor.ts (1)
85-91: Avoid double-traversing type arguments (risk of duplicate refs).Rely on the generic traversal; remove explicit recursive calls here.
- // Also extract dependencies from type arguments - const typeArguments = typeNode.getTypeArguments() - for (const typeArg of typeArguments) { - const argReferences = this.extractTypeReferences(typeArg) - references.push(...argReferences) - } ... - // Also extract dependencies from type arguments - const typeArguments = typeNode.getTypeArguments() - for (const typeArg of typeArguments) { - const argReferences = this.extractTypeReferences(typeArg) - references.push(...argReferences) - }Also applies to: 104-109
🧹 Nitpick comments (31)
src/traverse/types.ts (1)
10-10: Alias field on TraversedNode — LGTM; add brief JSDoc for clarityLooks good. Consider a short JSDoc describing when aliasName is set (only for imported, aliased symbols) and that generators should emit alias bindings when used.
tests/traverse/dependency-ordering.test.ts (1)
2-2: Good: reset shared resolver state per testImporting resolverStore and clearing it in beforeEach improves isolation for this singleton.
If tests run in parallel across files, consider serializing suites that touch the singleton or allow DependencyTraversal to accept an injected ResolverStore instance for test-local isolation.
Also applies to: 12-12
tests/import-resolution.test.ts (1)
1-1: ResolverStore reset added — LGTMPre-test clear avoids cross-test bleed from cached mappings.
If you later inject the store into traversers/printers, tests won’t need to import the singleton directly.
Also applies to: 11-11
tests/traverse/dependency-traversal.integration.test.ts (1)
2-2: Per-test resolver reset — LGTMSolid improvement for deterministic runs with a caching store.
Consider constructor injection of the store into DependencyTraversal to remove global coupling and make tests fully hermetic.
Also applies to: 15-15
src/parsers/parse-enums.ts (1)
6-7: Prefer nullish coalescing and guard alias validity for identifiers.Use ?? to avoid treating empty strings as falsy; additionally, ensure aliasName is a valid TS identifier to prevent emitting invalid code.
- parse(enumDeclaration: EnumDeclaration, aliasName?: string): void { - const enumName = aliasName || enumDeclaration.getName() + parse(enumDeclaration: EnumDeclaration, aliasName?: string): void { + const proposed = aliasName ?? enumDeclaration.getName() + const enumName = ts.isIdentifierText(proposed) ? proposed : enumDeclaration.getName()Add import (outside this hunk) if not present:
import { EnumDeclaration, VariableDeclarationKind, ts } from 'ts-morph'src/traverse/dependency-extractor.ts (2)
19-21: Avoid O(n) array materialization each loop iteration.Iterating a Set via Array.from(...) on every pass is unnecessary.
- const currentNodeId = Array.from(nodesToProcess)[0] + const iter = nodesToProcess.values() + const currentNodeId = iter.next().value as string | undefined
48-55: Dependency gating may drop legitimate edges if ResolverStore isn’t pre-seeded.If typeReferences already return qualified names, the resolverStore.hasQualifiedName() check is redundant and can cause missed deps before seeding. Ensure seeding occurs before extractDependencies(), or drop the extra gate.
- if (resolverStore.hasQualifiedName(referencedType) && nodeGraph.hasNode(referencedType)) { + if (nodeGraph.hasNode(referencedType)) {src/parsers/parse-function-declarations.ts (1)
8-13: Use nullish coalescing and ensure alias produces a valid identifier.Prevents empty-string aliases and invalid identifiers from breaking codegen.
- parse(functionDecl: FunctionDeclaration, aliasName?: string): void { - const originalFunctionName = functionDecl.getName() - if (!originalFunctionName) return - - const functionName = aliasName || originalFunctionName + parse(functionDecl: FunctionDeclaration, aliasName?: string): void { + const originalFunctionName = functionDecl.getName() + if (!originalFunctionName) return + const proposed = aliasName ?? originalFunctionName + const functionName = ts.isIdentifierText(proposed) ? proposed : originalFunctionNamesrc/handlers/typebox/type-reference-handler.ts (1)
12-38: Optional: support qualified names (namespaces) instead of falling back to Any.Handling ts.QualifiedName would improve coverage for namespaced types.
- if (Node.isIdentifier(referencedType)) { + if (Node.isIdentifier(referencedType)) { ... - } + } else if (Node.isQualifiedName(referencedType)) { + const right = referencedType.getRight().getText() + const typeName = resolverStore.resolveAliasName(right) ?? right + const typeBoxArgs = typeArguments.map((arg) => getTypeBoxType(arg)) + return typeArguments.length + ? ts.factory.createCallExpression(ts.factory.createIdentifier(typeName), undefined, typeBoxArgs) + : ts.factory.createIdentifier(typeName) + }src/traverse/local-type-collector.ts (1)
55-58: Align sourceFile used in addTypeMapping with the declaration’s sourceUse the declaration’s sourceFile for consistency (matches interfaces/enums/functions) and to avoid future refactor pitfalls.
// Add to ResolverStore during traversal resolverStore.addTypeMapping({ originalName: typeName, - sourceFile, + sourceFile: typeAlias.getSourceFile(), })src/utils/graph-visualizer.ts (1)
80-84: Show alias in labels for imported nodesImproves readability when aliases are used.
- label: `${nodeData.type}: ${nodeData.originalName}`, + label: nodeData.aliasName + ? `${nodeData.type}: ${nodeData.originalName} as ${nodeData.aliasName}` + : `${nodeData.type}: ${nodeData.originalName}`,src/traverse/import-collector.ts (2)
116-137: Propagate alias for functions to keep parity with typesCarry alias into nodeGraph and resolverStore for functions too.
for (const functionDecl of functions) { const functionName = functionDecl.getName() if (!functionName) continue const qualifiedName = resolverStore.generateQualifiedName( functionName, functionDecl.getSourceFile(), ) + const aliasName = aliasMap.get(functionName) this.nodeGraph.addTypeNode(qualifiedName, { node: functionDecl, type: 'function', originalName: functionName, qualifiedName, isImported: true, isMainCode: false, + aliasName, }) // Add to ResolverStore during traversal resolverStore.addTypeMapping({ originalName: functionName, sourceFile: functionDecl.getSourceFile(), + aliasName, }) }
41-64: Scope check: collecting all declarations in imported files (exported or not)If the intent is to include only exported symbols from imported modules, filter by
isExported()or by the module’s export list to avoid graph bloat.Also applies to: 66-89, 91-114, 116-138
src/parsers/parse-interfaces.ts (1)
43-75: Uniform printing approach (optional)Both branches re-print nodes via
this.printer.printNode. Consider extracting a tiny helper to avoid drift and make future tweaks (e.g., comments/JsDoc preservation) one place.tests/traverse/ambiguous-name-resolution.test.ts (1)
55-81: Trim redundant assertion
toBe(formatWithPrettier(...))already guarantees the output containsUserProfile. The extratoContain('UserProfile')is redundant and can be removed to speed up failures.- expect(result).toContain('UserProfile') expect(result).toBe( formatWithPrettier(tests/traverse/type-reference-extractor.test.ts (2)
19-66: NodeGraph setup is unused by the extractor
TypeReferenceExtractorconsultsresolverStore, notnodeGraph. Keeping thenodeGraphscaffolding here adds noise. If not needed for future assertions, drop it.- let nodeGraph: NodeGraph @@ - nodeGraph = new NodeGraph() @@ - nodeGraph.addTypeNode(userQualifiedName, { + // (Optional) If not asserting graph behavior, this can be removed. + /* nodeGraph.addTypeNode(userQualifiedName, { @@ - }) + }) */ @@ - nodeGraph.addTypeNode(adminQualifiedName, { + /* nodeGraph.addTypeNode(adminQualifiedName, { @@ - }) + }) */
19-101: Strengthen assertions for type-alias test (optional)Consider asserting both qualified names are discovered to ensure alias mapping works end-to-end.
@@ - const references = extractor.extractTypeReferences(typeNode) - - expect(references.length).toBeGreaterThan(0) + const references = extractor.extractTypeReferences(typeNode) + expect(references).toEqual( + expect.arrayContaining([userQualifiedName, adminQualifiedName]), + )src/traverse/type-reference-extractor.ts (3)
133-134: Deduplicate references before returning.Prevents downstream duplicate edges with minimal cost.
- return references + return Array.from(new Set(references))
7-30: Broaden BUILT_IN_TYPES to skip common globals.Reduce false positives for ubiquitous types.
private static readonly BUILT_IN_TYPES = new Set([ 'Partial', @@ 'NoInfer', 'Awaited', + // Common globals + 'Promise', + 'Array', + 'ReadonlyArray', + 'Date', + 'Map', + 'Set', + 'WeakMap', + 'WeakSet', ])
145-149: Strengthen cache key with source file path.Prevents rare collisions across files when reusing the extractor.
- const cacheKey = `${typeName}:${node.getKind()}:${node.getStart()}` + const cacheKey = `${typeName}:${node.getSourceFile().getFilePath()}:${node.getKind()}:${node.getStart()}`src/traverse/dependency-traversal.ts (3)
11-11: Avoid hidden global state coupling with resolverStore.Importing the singleton here couples traversal to global mutable state. Either inject a ResolverStore instance into DependencyTraversal or ensure each run starts from a clean store (see comment on startTraversal).
25-31: Reset per-run state to prevent cross-run leakage.Because resolverStore is a singleton, multiple traversals in the same process can bleed mappings across runs. Consider clearing at the start of traversal (or injecting a scoped store).
Apply this minimal safeguard:
startTraversal(sourceFile: SourceFile): TraversedNode[] { - // Mark main source file nodes as main code + // Ensure clean resolver state for this traversal + resolverStore.clear() + // Mark main source file nodes as main code addLocalTypes(sourceFile, this.nodeGraph, this.maincodeNodeIds, this.requiredNodeIds)
49-51: Micro-optimization: use a Set for membership checks.requiredQualifiedNames.includes in a loop is O(n^2). Convert to a Set once.
-const requiredQualifiedNames = allQualifiedNames.filter((qualifiedName: string) => - this.requiredNodeIds.has(qualifiedName), -) +const requiredQualifiedNames = allQualifiedNames.filter((q: string) => this.requiredNodeIds.has(q)) +const requiredSet = new Set(requiredQualifiedNames) ... -: topologicalSort(this.nodeGraph).filter((nodeId: string) => - requiredQualifiedNames.includes(nodeId), - ) +: topologicalSort(this.nodeGraph).filter((nodeId: string) => requiredSet.has(nodeId))Also applies to: 56-58
tests/utils/resolver-store.test.ts (3)
250-252: Avoid order-coupled assertions.Map key order is insertion-dependent. Prefer order-agnostic checks.
-const originalNames = resolverStore.getOriginalNames() -expect(originalNames).toEqual(['User', 'Product']) +const originalNames = resolverStore.getOriginalNames() +expect(originalNames).toEqual(expect.arrayContaining(['User', 'Product'])) +expect(originalNames).toHaveLength(2)
279-293: Add an idempotency test for addTypeMapping.Current store will append duplicates in the originalName index if the same mapping is added twice. Add a test to lock desired behavior (ignore duplicates).
+ test('should be idempotent when adding the same mapping twice', () => { + const sourceFile = project.createSourceFile( + 'models/user.ts', + 'export type User = { id: string }', + ) + resolverStore.addTypeMapping({ originalName: 'User', sourceFile }) + resolverStore.addTypeMapping({ originalName: 'User', sourceFile }) + expect(resolverStore.size()).toBe(1) + const names = resolverStore.getOriginalNames() + expect(names).toEqual(['User']) + })I can update the store to enforce this (see resolver-store comment).
275-277: Consider alias collision coverage.Add a test where two different source files register the same alias to ensure the store’s behavior (last-write wins vs. scoped aliasing) is explicit.
I can add a test and implement scoped aliasing if needed (alias keyed by importer file).
src/utils/resolver-store.ts (5)
33-38: Qualified name stability vs. collision probability.The 32‑bit custom hash is compact but increases collision risk on large codebases. Consider a stronger hash (e.g., Node’s crypto.createHash('sha1').update(path).digest('base64url').slice(0, 10)) or incorporate the full file path when feasible.
40-45: Alias index is globally scoped; consider scoping by import site.aliasNameIndex currently maps alias → qualifiedName globally. The same alias across different importing files will clobber. If aliases are per-import-site, key by alias + importer file path.
This would require extending TypeMappingInput with importerFilePath and setting the alias index with
${aliasName}::${importerFilePath}.
136-161: DRY: duplicate APIs for qualified-name enumeration.getQualifiedNames() and getAllQualifiedNames() return the same data.
Unify by delegating:
getAllQualifiedNames(): string[] { - return Array.from(this.typeMappings.keys()) + return this.getQualifiedNames() }
5-9: Avoid retaining heavyweight SourceFile objects in the store.Holding ts-morph SourceFile instances can increase memory usage. Prefer storing sourceFilePath: string and pass SourceFile only when generating the qualified name.
I can draft a non-breaking change that keeps sourceFile optional and adds sourceFilePath to TypeMapping.
174-174: Singleton export is convenient but brittle.Consider exporting a factory or allowing consumers to inject a ResolverStore instance to improve testability and avoid global mutable state.
📜 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 (23)
src/handlers/typebox/type-reference-handler.ts(2 hunks)src/parsers/parse-enums.ts(1 hunks)src/parsers/parse-function-declarations.ts(1 hunks)src/parsers/parse-interfaces.ts(3 hunks)src/parsers/parse-type-aliases.ts(3 hunks)src/printer/typebox-printer.ts(1 hunks)src/traverse/dependency-extractor.ts(3 hunks)src/traverse/dependency-traversal.ts(3 hunks)src/traverse/import-collector.ts(4 hunks)src/traverse/local-type-collector.ts(7 hunks)src/traverse/type-reference-extractor.ts(7 hunks)src/traverse/types.ts(1 hunks)src/utils/generate-qualified-name.ts(0 hunks)src/utils/graph-visualizer.ts(2 hunks)src/utils/resolver-store.ts(1 hunks)tests/import-resolution.test.ts(2 hunks)tests/traverse/ambiguous-name-resolution.test.ts(1 hunks)tests/traverse/dependency-ordering.test.ts(2 hunks)tests/traverse/dependency-traversal.integration.test.ts(2 hunks)tests/traverse/maincode-filter.test.ts(2 hunks)tests/traverse/non-transitive-dependency.test.ts(2 hunks)tests/traverse/type-reference-extractor.test.ts(1 hunks)tests/utils/resolver-store.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/utils/generate-qualified-name.ts
🧰 Additional context used
🧬 Code graph analysis (15)
tests/traverse/dependency-traversal.integration.test.ts (1)
src/utils/resolver-store.ts (1)
resolverStore(174-174)
tests/traverse/dependency-ordering.test.ts (1)
src/utils/resolver-store.ts (1)
resolverStore(174-174)
tests/traverse/type-reference-extractor.test.ts (3)
src/traverse/node-graph.ts (1)
NodeGraph(8-38)src/traverse/type-reference-extractor.ts (1)
TypeReferenceExtractor(4-156)src/utils/resolver-store.ts (1)
resolverStore(174-174)
tests/import-resolution.test.ts (1)
src/utils/resolver-store.ts (1)
resolverStore(174-174)
tests/traverse/non-transitive-dependency.test.ts (1)
src/utils/resolver-store.ts (1)
resolverStore(174-174)
tests/utils/resolver-store.test.ts (1)
src/utils/resolver-store.ts (2)
resolverStore(174-174)TypeMappingInput(11-15)
src/handlers/typebox/type-reference-handler.ts (1)
src/utils/resolver-store.ts (1)
resolverStore(174-174)
src/traverse/dependency-extractor.ts (2)
src/traverse/type-reference-extractor.ts (1)
TypeReferenceExtractor(4-156)src/utils/resolver-store.ts (1)
resolverStore(174-174)
tests/traverse/ambiguous-name-resolution.test.ts (2)
src/utils/resolver-store.ts (1)
resolverStore(174-174)tests/utils.ts (3)
createSourceFile(12-14)generateFormattedCode(26-37)formatWithPrettier(16-24)
tests/traverse/maincode-filter.test.ts (1)
src/utils/resolver-store.ts (1)
resolverStore(174-174)
src/utils/graph-visualizer.ts (1)
src/utils/resolver-store.ts (1)
resolverStore(174-174)
src/traverse/type-reference-extractor.ts (1)
src/utils/resolver-store.ts (1)
resolverStore(174-174)
src/traverse/dependency-traversal.ts (4)
src/traverse/import-collector.ts (1)
ImportCollector(6-144)src/traverse/types.ts (1)
TraversedNode(3-11)src/traverse/local-type-collector.ts (1)
addLocalTypes(5-134)src/utils/resolver-store.ts (1)
resolverStore(174-174)
src/traverse/local-type-collector.ts (1)
src/utils/resolver-store.ts (1)
resolverStore(174-174)
src/traverse/import-collector.ts (1)
src/utils/resolver-store.ts (1)
resolverStore(174-174)
🔇 Additional comments (19)
tests/traverse/non-transitive-dependency.test.ts (1)
1-1: Good isolation for global ResolverStoreClearing the store in beforeEach is appropriate for a shared singleton.
Optionally add afterEach(() => resolverStore.clear()) for belt-and-suspenders if a test aborts mid-run.
Also applies to: 11-11
tests/traverse/maincode-filter.test.ts (1)
1-1: Isolation ensured: all test suites clear ResolverStore
Every test file invokesresolverStore.clear()at startup (e.g. tests/import-resolution.test.ts:11, tests/utils/resolver-store.test.ts:11 & 291, tests/traverse/*.test.ts), guaranteeing no shared state even if run in parallel.src/traverse/dependency-extractor.ts (2)
3-3: ResolverStore integration aligns with the PR’s centralization goal.
15-15: Constructor change looks good.Localizing cache/state in the extractor avoids accidental coupling with NodeGraph.
src/handlers/typebox/type-reference-handler.ts (1)
2-2: ResolverStore usage here is appropriate and consistent with the design.src/traverse/local-type-collector.ts (1)
23-34: Imports-only path ignores default/namespace imports—confirm intended scopeThis branch only considers named imports. If default (
import Foo from ...) or namespace (import * as NS from ...) imports should also seed requiredNodeIds, extend this logic; otherwise document the constraint.Also applies to: 30-31
src/traverse/import-collector.ts (1)
23-33: Default and namespace imports are ignored in alias mapping—confirmIf needed, also handle
getDefaultImport()andgetNamespaceImport()to capture aliases fromimport Foo fromandimport * as NS from.src/parsers/parse-interfaces.ts (2)
18-21: Good: generic vs. non-generic split is clearBranching on
typeParameters.lengthand delegating avoids duplication and keeps responsibilities clean.
24-41: OK: alias-driven emission pathPassing
interfaceNamethrough to TypeBox var statement and static alias generation aligns with alias propagation. No issues.tests/traverse/ambiguous-name-resolution.test.ts (1)
126-166: Solid golden testEnd-to-end expectation for composed interfaces looks good and exercises alias emission.
src/printer/typebox-printer.ts (2)
40-57: Forwarding aliasName to parsers looks correct.Consistently threads aliasName through all parser calls; matches the intent of alias-aware codegen.
39-57: Parser.parse() APIs updated with optional aliasName
Confirmed that TypeAliasParser, InterfaceParser, FunctionDeclarationParser, and EnumParser signatures includealiasName?: string, and all call sites intypebox-printer.tspass thealiasNameargument.src/traverse/type-reference-extractor.ts (1)
151-155: Pass origin context to resolverStore (if supported).If multiple distinct types share the same original name, resolving by name alone is ambiguous; prefer APIs that accept the referencing source file or module context.
If available, switch to something like:
const qualifiedName = resolverStore.resolveQualifiedName(typeName, node.getSourceFile().getFilePath())Otherwise, confirm resolverStore returns null on ambiguity and that callers properly handle missing deps.
src/parsers/parse-type-aliases.ts (4)
12-14: Processed-types keyed by effective name is correct.Prevents duplicate emissions when aliasName overrides are applied.
19-22: Generic vs regular path now respects aliasName.Good use of the resolved typeName for both branches.
25-37: Regular alias path looks good; confirm addStaticTypeAlias uses the effective name.No issues spotted; relies on shared utils consistently.
If downstream tools assume original names, ensure they reference the emitted alias-bound identifiers.
39-67: Generic alias path looks good; prints function and static alias with the effective name.Consistent with GenericTypeUtils usage.
src/traverse/dependency-traversal.ts (1)
60-64: Verify completeness: ensure all required nodes are registered in resolverStore.If any required node is missing from resolverStore (e.g., imported functions not capturing aliases), it will be dropped here and silently filtered out.
Would you like me to scan import-collector for parity (types vs functions) and add alias support for function imports as well?
tests/utils/resolver-store.test.ts (1)
9-12: Good isolation.Creating a fresh Project and clearing resolverStore per test prevents cross-test contamination.
| const originalTypeName = referencedType.getText() | ||
|
|
||
| // Use the ResolverStore to get the alias name if available | ||
| const typeName = resolverStore.resolveAliasName(originalTypeName) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a safe fallback when no alias is registered.
Ensure we never emit an undefined identifier.
- // Use the ResolverStore to get the alias name if available
- const typeName = resolverStore.resolveAliasName(originalTypeName)
+ // Use the ResolverStore to get the alias name if available
+ const typeName = resolverStore.resolveAliasName(originalTypeName) ?? originalTypeName📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const originalTypeName = referencedType.getText() | |
| // Use the ResolverStore to get the alias name if available | |
| const typeName = resolverStore.resolveAliasName(originalTypeName) | |
| const originalTypeName = referencedType.getText() | |
| // Use the ResolverStore to get the alias name if available | |
| const typeName = resolverStore.resolveAliasName(originalTypeName) ?? originalTypeName |
🤖 Prompt for AI Agents
In src/handlers/typebox/type-reference-handler.ts around lines 17 to 21,
resolverStore.resolveAliasName(originalTypeName) can return undefined and may
cause an undefined identifier to be emitted; update the logic to provide a safe
fallback (e.g., use originalTypeName when no alias is registered, or a sanitized
default identifier) so that typeName is never undefined and downstream code
always receives a valid identifier.
| parse(typeAlias: TypeAliasDeclaration, aliasName?: string): void { | ||
| const typeName = aliasName || typeAlias.getName() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sanitize aliasName to a valid TS identifier before use.
Aliases derived from paths may include invalid chars, breaking codegen.
- parse(typeAlias: TypeAliasDeclaration, aliasName?: string): void {
- const typeName = aliasName || typeAlias.getName()
+ parse(typeAlias: TypeAliasDeclaration, aliasName?: string): void {
+ const makeSafeIdentifier = (name: string) => name.replace(/[^A-Za-z0-9_$]/g, '_')
+ const resolved = aliasName ?? typeAlias.getName()
+ const typeName = ts.isIdentifierText(resolved, ts.ScriptTarget.Latest)
+ ? resolved
+ : makeSafeIdentifier(resolved)📝 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.
| parse(typeAlias: TypeAliasDeclaration, aliasName?: string): void { | |
| const typeName = aliasName || typeAlias.getName() | |
| parse(typeAlias: TypeAliasDeclaration, aliasName?: string): void { | |
| const makeSafeIdentifier = (name: string) => name.replace(/[^A-Za-z0-9_$]/g, '_') | |
| const resolved = aliasName ?? typeAlias.getName() | |
| const typeName = ts.isIdentifierText(resolved, ts.ScriptTarget.Latest) | |
| ? resolved | |
| : makeSafeIdentifier(resolved) |
🤖 Prompt for AI Agents
In src/parsers/parse-type-aliases.ts around lines 9–11, aliasName may contain
characters invalid in a TypeScript identifier (e.g., derived from paths);
sanitize aliasName before using it as typeName by: normalize aliasName to a safe
identifier (replace any character not in [A-Za-z0-9_$] with underscore), ensure
it does not start with a digit (prefix with '_' if it does), fall back to
typeAlias.getName() when aliasName is missing or becomes empty after
sanitization, and use that sanitized value as typeName so generated code never
contains invalid identifiers.
| const requiredQualifiedNames = allQualifiedNames.filter((qualifiedName: string) => | ||
| this.requiredNodeIds.has(qualifiedName), | ||
| ) | ||
|
|
||
| // Apply topological ordering only to the required nodes | ||
| const orderedNodes = hasCycle(this.nodeGraph) | ||
| ? requiredQualifiedNames | ||
| : topologicalSort(this.nodeGraph).filter((nodeId: string) => | ||
| requiredQualifiedNames.includes(nodeId), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Topological sort should be based on the required subgraph, not global cycle presence.
A cycle elsewhere in the graph disables ordering for an acyclic required subset, degrading output determinism.
One pragmatic fix is to attempt sorting and fall back on failure:
- const orderedNodes = hasCycle(this.nodeGraph)
- ? requiredQualifiedNames
- : topologicalSort(this.nodeGraph).filter((nodeId: string) =>
- requiredQualifiedNames.includes(nodeId),
- )
+ let orderedNodes: string[]
+ try {
+ const requiredSet = new Set(requiredQualifiedNames)
+ orderedNodes = topologicalSort(this.nodeGraph).filter((id: string) => requiredSet.has(id))
+ } catch {
+ // Graph has cycles; fall back to required insertion order
+ orderedNodes = requiredQualifiedNames
+ }If feasible, prefer sorting an induced subgraph of required nodes for correctness and performance. I can sketch a small helper to build that subgraph if NodeGraph exposes the underlying Graphology graph.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const requiredQualifiedNames = allQualifiedNames.filter((qualifiedName: string) => | |
| this.requiredNodeIds.has(qualifiedName), | |
| ) | |
| // Apply topological ordering only to the required nodes | |
| const orderedNodes = hasCycle(this.nodeGraph) | |
| ? requiredQualifiedNames | |
| : topologicalSort(this.nodeGraph).filter((nodeId: string) => | |
| requiredQualifiedNames.includes(nodeId), | |
| ) | |
| const requiredQualifiedNames = allQualifiedNames.filter((qualifiedName: string) => | |
| this.requiredNodeIds.has(qualifiedName), | |
| ) | |
| // Apply topological ordering only to the required nodes | |
| - const orderedNodes = hasCycle(this.nodeGraph) | |
| - ? requiredQualifiedNames | |
| - : topologicalSort(this.nodeGraph).filter((nodeId: string) => | |
| - requiredQualifiedNames.includes(nodeId), | |
| let orderedNodes: string[] | |
| try { | |
| // Attempt to sort the full graph, then filter to required nodes | |
| const requiredSet = new Set(requiredQualifiedNames) | |
| orderedNodes = topologicalSort(this.nodeGraph).filter((id: string) => | |
| requiredSet.has(id), | |
| ) | |
| } catch { | |
| // If sorting fails (due to cycles), fall back to original insertion order | |
| orderedNodes = requiredQualifiedNames | |
| } |
🤖 Prompt for AI Agents
In src/traverse/dependency-traversal.ts around lines 49 to 58, the code
currently disables topological ordering when any cycle exists in the global
graph; instead create an induced subgraph containing only requiredQualifiedNames
(i.e., nodes in this.requiredNodeIds and their edges between those nodes) and
run topologicalSort against that subgraph; if the induced subgraph has a cycle
or topologicalSort throws, fall back to the original requiredQualifiedNames
order—do not rely on hasCycle(this.nodeGraph) for this decision. Ensure you
either use a helper to build the induced graph from NodeGraph's underlying graph
or filter edges appropriately, then replace the global-cycle check with a
cycle/sort check on the induced subgraph and a safe fallback.
| const typeNameNode = node.getTypeName() | ||
| const typeName = typeNameNode.getText() | ||
|
|
||
| for (const qualifiedName of this.nodeGraph.nodes()) { | ||
| const nodeData = this.nodeGraph.getNode(qualifiedName) | ||
| if (nodeData.originalName === typeName) { | ||
| references.push(qualifiedName) | ||
| break | ||
| } | ||
| const qualifiedName = this.resolveTypeNameToQualifiedName(typeName, typeNameNode) | ||
| if (qualifiedName) { | ||
| references.push(qualifiedName) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix QualifiedName resolution to use the rightmost identifier.
Passing "ns.Type" to resolverStore likely fails; use "Type".
Apply:
- const typeNameNode = node.getTypeName()
- const typeName = typeNameNode.getText()
+ const typeNameNode = node.getTypeName()
+ const typeName = Node.isQualifiedName(typeNameNode)
+ ? typeNameNode.getRight().getText()
+ : typeNameNode.getText()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const typeNameNode = node.getTypeName() | |
| const typeName = typeNameNode.getText() | |
| for (const qualifiedName of this.nodeGraph.nodes()) { | |
| const nodeData = this.nodeGraph.getNode(qualifiedName) | |
| if (nodeData.originalName === typeName) { | |
| references.push(qualifiedName) | |
| break | |
| } | |
| const qualifiedName = this.resolveTypeNameToQualifiedName(typeName, typeNameNode) | |
| if (qualifiedName) { | |
| references.push(qualifiedName) | |
| } | |
| const typeNameNode = node.getTypeName() | |
| const typeName = Node.isQualifiedName(typeNameNode) | |
| ? typeNameNode.getRight().getText() | |
| : typeNameNode.getText() | |
| const qualifiedName = this.resolveTypeNameToQualifiedName(typeName, typeNameNode) | |
| if (qualifiedName) { | |
| references.push(qualifiedName) | |
| } |
🤖 Prompt for AI Agents
In src/traverse/type-reference-extractor.ts around lines 41-47, the code passes
the full dotted name (e.g. "ns.Type") to resolveTypeNameToQualifiedName which
breaks resolution; extract the rightmost identifier ("Type") before calling the
resolver. Concretely, detect if typeNameNode is a qualified name and use its
rightmost identifier node/text (or otherwise the identifier text) and pass that
simple name (and its identifier node) into resolveTypeNameToQualifiedName so
resolverStore receives only the actual type identifier rather than the
namespace-qualified string.
| 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 | ||
| } | ||
| const qualifiedName = this.resolveTypeNameToQualifiedName(typeName, exprName) | ||
| if (qualifiedName) { | ||
| references.push(qualifiedName) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Do the same for typeof (TypeQuery) names.
QualifiedName should be reduced to its rightmost part.
- if (Node.isIdentifier(exprName) || Node.isQualifiedName(exprName)) {
- const typeName = exprName.getText()
+ if (Node.isIdentifier(exprName) || Node.isQualifiedName(exprName)) {
+ const typeName = Node.isQualifiedName(exprName)
+ ? exprName.getRight().getText()
+ : exprName.getText()Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/traverse/type-reference-extractor.ts around lines 54-61, extend the
existing Identifier/QualifiedName handling to also handle TypeQuery (typeof)
names: detect when the node is a TypeQuery (or when exprName comes from a
TypeQuery), obtain its exprName, and treat it like the other name cases; when
exprName is a QualifiedName reduce it to its rightmost part (the simple
identifier) before calling resolveTypeNameToQualifiedName (e.g., use the
rightmost identifier text or split on '.' and take last segment), then call
this.resolveTypeNameToQualifiedName with that reduced name and push the resolved
qualifiedName into references if present.
| // Handle call expressions (for generic type calls like EntityInfo(PropertyId)) | ||
| if (Node.isCallExpression(node)) { | ||
| const expression = node.getExpression() | ||
|
|
||
| if (Node.isIdentifier(expression)) { | ||
| const typeName = expression.getText() | ||
|
|
||
| for (const qualifiedName of this.nodeGraph.nodes()) { | ||
| const nodeData = this.nodeGraph.getNode(qualifiedName) | ||
| if (nodeData.originalName === typeName) { | ||
| references.push(qualifiedName) | ||
| break | ||
| } | ||
| const qualifiedName = this.resolveTypeNameToQualifiedName(typeName, expression) | ||
| if (qualifiedName) { | ||
| references.push(qualifiedName) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
CallExpression: also support PropertyAccessExpression.
Handles calls like ns.EntityInfo(PropertyId).
- if (Node.isCallExpression(node)) {
+ if (Node.isCallExpression(node)) {
const expression = node.getExpression()
- if (Node.isIdentifier(expression)) {
- const typeName = expression.getText()
+ if (Node.isIdentifier(expression) || Node.isPropertyAccessExpression(expression)) {
+ const typeName = Node.isPropertyAccessExpression(expression)
+ ? expression.getName()
+ : expression.getText()📝 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.
| // Handle call expressions (for generic type calls like EntityInfo(PropertyId)) | |
| if (Node.isCallExpression(node)) { | |
| const expression = node.getExpression() | |
| if (Node.isIdentifier(expression)) { | |
| const typeName = expression.getText() | |
| for (const qualifiedName of this.nodeGraph.nodes()) { | |
| const nodeData = this.nodeGraph.getNode(qualifiedName) | |
| if (nodeData.originalName === typeName) { | |
| references.push(qualifiedName) | |
| break | |
| } | |
| const qualifiedName = this.resolveTypeNameToQualifiedName(typeName, expression) | |
| if (qualifiedName) { | |
| references.push(qualifiedName) | |
| } | |
| } | |
| } | |
| // Handle call expressions (for generic type calls like EntityInfo(PropertyId)) | |
| if (Node.isCallExpression(node)) { | |
| const expression = node.getExpression() | |
| if (Node.isIdentifier(expression) || Node.isPropertyAccessExpression(expression)) { | |
| const typeName = Node.isPropertyAccessExpression(expression) | |
| ? expression.getName() | |
| : expression.getText() | |
| const qualifiedName = this.resolveTypeNameToQualifiedName(typeName, expression) | |
| if (qualifiedName) { | |
| references.push(qualifiedName) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/traverse/type-reference-extractor.ts around lines 115-126, the
CallExpression handling only checks for Identifier expressions but needs to also
accept PropertyAccessExpression (to handle calls like
ns.EntityInfo(PropertyId)). Change the conditional to allow
Node.isIdentifier(expression) || Node.isPropertyAccessExpression(expression),
compute the typeName from expression.getText() (works for both kinds), and call
this.resolveTypeNameToQualifiedName(typeName, expression) as before; if a
qualified name is returned, push it into references. Ensure the expression
passed to resolveTypeNameToQualifiedName is the actual PropertyAccessExpression
when present so namespace-qualified names are resolved correctly.
| // Convert nodes using resolver store to get all qualified names | ||
| const allQualifiedNames = resolverStore.getAllQualifiedNames() | ||
| for (const nodeId of allQualifiedNames) { | ||
| // Only include nodes that exist in the graph | ||
| if (graph.hasNode(nodeId)) { | ||
| const nodeData = graph.getNodeAttributes(nodeId) | ||
|
|
||
| const node: GraphNode = { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid coupling to resolverStore for node enumeration—use union with graph.nodes()
Relying solely on resolverStore can omit graph-only nodes. Iterate the union to stay robust.
- // Convert nodes using resolver store to get all qualified names
- const allQualifiedNames = resolverStore.getAllQualifiedNames()
- for (const nodeId of allQualifiedNames) {
- // Only include nodes that exist in the graph
- if (graph.hasNode(nodeId)) {
- const nodeData = graph.getNodeAttributes(nodeId)
+ // Use union of graph nodes and resolver store names
+ const nodeIds = new Set<string>([...graph.nodes(), ...resolverStore.getAllQualifiedNames()])
+ for (const nodeId of nodeIds) {
+ if (!graph.hasNode(nodeId)) continue
+ const nodeData = graph.getNodeAttributes(nodeId)📝 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.
| // Convert nodes using resolver store to get all qualified names | |
| const allQualifiedNames = resolverStore.getAllQualifiedNames() | |
| for (const nodeId of allQualifiedNames) { | |
| // Only include nodes that exist in the graph | |
| if (graph.hasNode(nodeId)) { | |
| const nodeData = graph.getNodeAttributes(nodeId) | |
| const node: GraphNode = { | |
| // Use union of graph nodes and resolver store names | |
| const nodeIds = new Set<string>([...graph.nodes(), ...resolverStore.getAllQualifiedNames()]) | |
| for (const nodeId of nodeIds) { | |
| if (!graph.hasNode(nodeId)) continue | |
| const nodeData = graph.getNodeAttributes(nodeId) | |
| const node: GraphNode = { |
🤖 Prompt for AI Agents
In src/utils/graph-visualizer.ts around lines 72 to 79, the code enumerates
nodes using resolverStore.getAllQualifiedNames(), which can miss nodes that
exist only in the graph; replace that single-source enumeration with a union of
resolverStore.getAllQualifiedNames() and graph.nodes() (or the graph API that
returns all node IDs) by creating a Set combining both sources, then iterate the
Set so every node from either source is visited; keep the existing graph.hasNode
and graph.getNodeAttributes checks/logic when constructing GraphNode.
| addTypeMapping(mapping: TypeMappingInput): void { | ||
| const qualifiedName = this.generateQualifiedName(mapping.originalName, mapping.sourceFile) | ||
| const typeMapping: TypeMapping = { | ||
| originalName: mapping.originalName, | ||
| qualifiedName, | ||
| aliasName: mapping.aliasName, | ||
| sourceFile: mapping.sourceFile, | ||
| } | ||
|
|
||
| this.typeMappings.set(qualifiedName, typeMapping) | ||
|
|
||
| // Update originalName index to support multiple qualified names per original name | ||
| const existingQualifiedNames = this.originalNameIndex.get(mapping.originalName) || [] | ||
| existingQualifiedNames.push(qualifiedName) | ||
| this.originalNameIndex.set(mapping.originalName, existingQualifiedNames) | ||
|
|
||
| if (mapping.aliasName) { | ||
| this.aliasNameIndex.set(mapping.aliasName, qualifiedName) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make addTypeMapping idempotent and prevent index duplication.
Repeated additions of the same mapping re-push into originalNameIndex and can overwrite alias mappings.
addTypeMapping(mapping: TypeMappingInput): void {
const qualifiedName = this.generateQualifiedName(mapping.originalName, mapping.sourceFile)
+ // Idempotency: if we already have this qualified name, bail early
+ if (this.typeMappings.has(qualifiedName)) return
const typeMapping: TypeMapping = {
originalName: mapping.originalName,
qualifiedName,
aliasName: mapping.aliasName,
sourceFile: mapping.sourceFile,
}
this.typeMappings.set(qualifiedName, typeMapping)
// Update originalName index to support multiple qualified names per original name
- const existingQualifiedNames = this.originalNameIndex.get(mapping.originalName) || []
- existingQualifiedNames.push(qualifiedName)
- this.originalNameIndex.set(mapping.originalName, existingQualifiedNames)
+ const arr = this.originalNameIndex.get(mapping.originalName) || []
+ if (!arr.includes(qualifiedName)) arr.push(qualifiedName)
+ this.originalNameIndex.set(mapping.originalName, arr)
if (mapping.aliasName) {
this.aliasNameIndex.set(mapping.aliasName, qualifiedName)
}
}📝 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.
| addTypeMapping(mapping: TypeMappingInput): void { | |
| const qualifiedName = this.generateQualifiedName(mapping.originalName, mapping.sourceFile) | |
| const typeMapping: TypeMapping = { | |
| originalName: mapping.originalName, | |
| qualifiedName, | |
| aliasName: mapping.aliasName, | |
| sourceFile: mapping.sourceFile, | |
| } | |
| this.typeMappings.set(qualifiedName, typeMapping) | |
| // Update originalName index to support multiple qualified names per original name | |
| const existingQualifiedNames = this.originalNameIndex.get(mapping.originalName) || [] | |
| existingQualifiedNames.push(qualifiedName) | |
| this.originalNameIndex.set(mapping.originalName, existingQualifiedNames) | |
| if (mapping.aliasName) { | |
| this.aliasNameIndex.set(mapping.aliasName, qualifiedName) | |
| } | |
| } | |
| addTypeMapping(mapping: TypeMappingInput): void { | |
| const qualifiedName = this.generateQualifiedName(mapping.originalName, mapping.sourceFile) | |
| // Idempotency: if we already have this qualified name, bail early | |
| if (this.typeMappings.has(qualifiedName)) return | |
| const typeMapping: TypeMapping = { | |
| originalName: mapping.originalName, | |
| qualifiedName, | |
| aliasName: mapping.aliasName, | |
| sourceFile: mapping.sourceFile, | |
| } | |
| this.typeMappings.set(qualifiedName, typeMapping) | |
| // Update originalName index to support multiple qualified names per original name | |
| const arr = this.originalNameIndex.get(mapping.originalName) || [] | |
| if (!arr.includes(qualifiedName)) arr.push(qualifiedName) | |
| this.originalNameIndex.set(mapping.originalName, arr) | |
| if (mapping.aliasName) { | |
| this.aliasNameIndex.set(mapping.aliasName, qualifiedName) | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/utils/resolver-store.ts around lines 67 to 86, addTypeMapping currently
re-pushes the same qualifiedName into originalNameIndex and unconditionally
overwrites aliasNameIndex on repeated calls; make it idempotent by first
checking if this.typeMappings already has the qualifiedName and returning early
if so, only push qualifiedName into existingQualifiedNames when it is not
already present, and only set aliasNameIndex if mapping.aliasName is provided
and the alias is not already present (or already maps to the same
qualifiedName); do not overwrite an existing alias that points to a different
qualifiedName.
| getTypeMappingByOriginalName(originalName: string): TypeMapping | null { | ||
| const qualifiedNames = this.originalNameIndex.get(originalName) | ||
| if (!qualifiedNames || qualifiedNames.length === 0) return null | ||
| // Return the first mapping for now - could be enhanced for better disambiguation | ||
| const firstQualifiedName = qualifiedNames[0] | ||
| return firstQualifiedName ? this.typeMappings.get(firstQualifiedName) || null : null | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Original-name lookup is ambiguous; expose a disambiguated API.
Returning the first mapping can produce incorrect resolutions when the same name exists in multiple files.
Add a scoped API:
+ /**
+ * Return all mappings for an original name (disambiguation by caller).
+ */
+ getTypeMappingsByOriginalName(originalName: string): TypeMapping[] {
+ const qualifiedNames = this.originalNameIndex.get(originalName) || []
+ return qualifiedNames
+ .map((q) => this.typeMappings.get(q))
+ .filter((m): m is TypeMapping => !!m)
+ }
+
+ /**
+ * Optionally resolve by providing the declaring sourceFile path.
+ */
+ getTypeMappingByOriginalName(
+ originalName: string,
+ declaringFilePath?: string,
+ ): TypeMapping | null {
+ const qualifiedNames = this.originalNameIndex.get(originalName)
+ if (!qualifiedNames || qualifiedNames.length === 0) return null
+ if (!declaringFilePath) return this.typeMappings.get(qualifiedNames[0]) || null
+ const match = qualifiedNames.find((q) => this.typeMappings.get(q)?.sourceFile?.getFilePath() === declaringFilePath)
+ return match ? this.typeMappings.get(match) || null : null
+ }Consumers like type-reference resolution can pass context to pick the correct mapping.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/utils/resolver-store.ts around lines 88-95, the current
getTypeMappingByOriginalName returns the first match and is ambiguous when the
same original name exists in multiple files; add a disambiguated API: implement
getTypeMappingsByOriginalName(originalName: string): TypeMapping[] to return all
matches (never drop duplicates), and implement
getTypeMappingByOriginalNameInScope(originalName: string, scope: {filePath?:
string; namespace?: string}): TypeMapping | null that filters the full-match
list by scope (prefer exact filePath, then namespace, then fallback to single
match or null); update the existing callers (e.g., type-reference resolution) to
use the scoped variant when context is available and keep the old method
delegating to the new getTypeMappingsByOriginalName for backward compatibility.
| test('should disambiguate type references when multiple files have same interface name', () => { | ||
| // Create two files with same interface name | ||
| const commonFile = project.createSourceFile( | ||
| 'common-types.ts', | ||
| ` | ||
| export interface BaseEntity { | ||
| id: string; | ||
| createdAt: Date; | ||
| } | ||
| `, | ||
| ) | ||
|
|
||
| const legacyFile = project.createSourceFile( | ||
| 'legacy-types.ts', | ||
| ` | ||
| export interface BaseEntity { | ||
| entityId: number; | ||
| version: number; | ||
| } | ||
| `, | ||
| ) | ||
|
|
||
| // Add both BaseEntity interfaces to the node graph | ||
| const commonInterface = commonFile.getInterfaces()[0]! | ||
| const legacyInterface = legacyFile.getInterfaces()[0]! | ||
|
|
||
| const commonQualifiedName = resolverStore.generateQualifiedName('BaseEntity', commonFile) | ||
| const legacyQualifiedName = resolverStore.generateQualifiedName('BaseEntity', legacyFile) | ||
|
|
||
| nodeGraph.addTypeNode(commonQualifiedName, { | ||
| node: commonInterface, | ||
| type: 'interface', | ||
| originalName: 'BaseEntity', | ||
| qualifiedName: commonQualifiedName, | ||
| isImported: true, | ||
| isMainCode: false, | ||
| }) | ||
|
|
||
| nodeGraph.addTypeNode(legacyQualifiedName, { | ||
| node: legacyInterface, | ||
| type: 'interface', | ||
| originalName: 'BaseEntity', | ||
| qualifiedName: legacyQualifiedName, | ||
| isImported: true, | ||
| isMainCode: false, | ||
| }) | ||
|
|
||
| // Create main file with interface inheritance | ||
| const mainFile = project.createSourceFile( | ||
| 'main.ts', | ||
| ` | ||
| import { BaseEntity as CommonBase } from "./common-types"; | ||
| import { BaseEntity as LegacyBase } from "./legacy-types"; | ||
| interface ModernUser extends CommonBase { | ||
| email: string; | ||
| } | ||
| interface LegacyUser extends LegacyBase { | ||
| username: string; | ||
| } | ||
| `, | ||
| ) | ||
|
|
||
| const modernUserInterface = mainFile.getInterfaces()[0]! | ||
| const legacyUserInterface = mainFile.getInterfaces()[1]! | ||
|
|
||
| // Extract references from both interfaces | ||
| const modernReferences = extractor.extractTypeReferences(modernUserInterface) | ||
| const legacyReferences = extractor.extractTypeReferences(legacyUserInterface) | ||
|
|
||
| expect(modernReferences.length).toBeGreaterThanOrEqual(0) | ||
| expect(legacyReferences.length).toBeGreaterThanOrEqual(0) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test 2 currently vacuous; add alias mappings and assert real resolution
expect(...).toBeGreaterThanOrEqual(0) always passes and you don’t add alias mappings for CommonBase/LegacyBase, so this test won’t catch regressions in resolver lookups.
Apply:
@@
test('should disambiguate type references when multiple files have same interface name', () => {
@@
- // Add both BaseEntity interfaces to the node graph
+ // Add both BaseEntity interfaces to the node graph
const commonInterface = commonFile.getInterfaces()[0]!
const legacyInterface = legacyFile.getInterfaces()[0]!
@@
const commonQualifiedName = resolverStore.generateQualifiedName('BaseEntity', commonFile)
const legacyQualifiedName = resolverStore.generateQualifiedName('BaseEntity', legacyFile)
@@
nodeGraph.addTypeNode(legacyQualifiedName, {
@@
})
+ // Map import aliases -> qualified names in the resolver
+ resolverStore.addTypeMapping({
+ originalName: 'BaseEntity',
+ sourceFile: commonFile,
+ aliasName: 'CommonBase',
+ })
+ resolverStore.addTypeMapping({
+ originalName: 'BaseEntity',
+ sourceFile: legacyFile,
+ aliasName: 'LegacyBase',
+ })
@@
- const modernReferences = extractor.extractTypeReferences(modernUserInterface)
- const legacyReferences = extractor.extractTypeReferences(legacyUserInterface)
-
- expect(modernReferences.length).toBeGreaterThanOrEqual(0)
- expect(legacyReferences.length).toBeGreaterThanOrEqual(0)
+ const modernReferences = extractor.extractTypeReferences(modernUserInterface)
+ const legacyReferences = extractor.extractTypeReferences(legacyUserInterface)
+
+ expect(modernReferences).toContain(commonQualifiedName)
+ expect(legacyReferences).toContain(legacyQualifiedName)
})📝 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.
| test('should disambiguate type references when multiple files have same interface name', () => { | |
| // Create two files with same interface name | |
| const commonFile = project.createSourceFile( | |
| 'common-types.ts', | |
| ` | |
| export interface BaseEntity { | |
| id: string; | |
| createdAt: Date; | |
| } | |
| `, | |
| ) | |
| const legacyFile = project.createSourceFile( | |
| 'legacy-types.ts', | |
| ` | |
| export interface BaseEntity { | |
| entityId: number; | |
| version: number; | |
| } | |
| `, | |
| ) | |
| // Add both BaseEntity interfaces to the node graph | |
| const commonInterface = commonFile.getInterfaces()[0]! | |
| const legacyInterface = legacyFile.getInterfaces()[0]! | |
| const commonQualifiedName = resolverStore.generateQualifiedName('BaseEntity', commonFile) | |
| const legacyQualifiedName = resolverStore.generateQualifiedName('BaseEntity', legacyFile) | |
| nodeGraph.addTypeNode(commonQualifiedName, { | |
| node: commonInterface, | |
| type: 'interface', | |
| originalName: 'BaseEntity', | |
| qualifiedName: commonQualifiedName, | |
| isImported: true, | |
| isMainCode: false, | |
| }) | |
| nodeGraph.addTypeNode(legacyQualifiedName, { | |
| node: legacyInterface, | |
| type: 'interface', | |
| originalName: 'BaseEntity', | |
| qualifiedName: legacyQualifiedName, | |
| isImported: true, | |
| isMainCode: false, | |
| }) | |
| // Create main file with interface inheritance | |
| const mainFile = project.createSourceFile( | |
| 'main.ts', | |
| ` | |
| import { BaseEntity as CommonBase } from "./common-types"; | |
| import { BaseEntity as LegacyBase } from "./legacy-types"; | |
| interface ModernUser extends CommonBase { | |
| email: string; | |
| } | |
| interface LegacyUser extends LegacyBase { | |
| username: string; | |
| } | |
| `, | |
| ) | |
| const modernUserInterface = mainFile.getInterfaces()[0]! | |
| const legacyUserInterface = mainFile.getInterfaces()[1]! | |
| // Extract references from both interfaces | |
| const modernReferences = extractor.extractTypeReferences(modernUserInterface) | |
| const legacyReferences = extractor.extractTypeReferences(legacyUserInterface) | |
| expect(modernReferences.length).toBeGreaterThanOrEqual(0) | |
| expect(legacyReferences.length).toBeGreaterThanOrEqual(0) | |
| }) | |
| test('should disambiguate type references when multiple files have same interface name', () => { | |
| // Create two files with same interface name | |
| const commonFile = project.createSourceFile( | |
| 'common-types.ts', | |
| ` | |
| export interface BaseEntity { | |
| id: string; | |
| createdAt: Date; | |
| } | |
| `, | |
| ) | |
| const legacyFile = project.createSourceFile( | |
| 'legacy-types.ts', | |
| ` | |
| export interface BaseEntity { | |
| entityId: number; | |
| version: number; | |
| } | |
| `, | |
| ) | |
| // Add both BaseEntity interfaces to the node graph | |
| const commonInterface = commonFile.getInterfaces()[0]! | |
| const legacyInterface = legacyFile.getInterfaces()[0]! | |
| const commonQualifiedName = resolverStore.generateQualifiedName('BaseEntity', commonFile) | |
| const legacyQualifiedName = resolverStore.generateQualifiedName('BaseEntity', legacyFile) | |
| nodeGraph.addTypeNode(commonQualifiedName, { | |
| node: commonInterface, | |
| type: 'interface', | |
| originalName: 'BaseEntity', | |
| qualifiedName: commonQualifiedName, | |
| isImported: true, | |
| isMainCode: false, | |
| }) | |
| nodeGraph.addTypeNode(legacyQualifiedName, { | |
| node: legacyInterface, | |
| type: 'interface', | |
| originalName: 'BaseEntity', | |
| qualifiedName: legacyQualifiedName, | |
| isImported: true, | |
| isMainCode: false, | |
| }) | |
| // Map import aliases -> qualified names in the resolver | |
| resolverStore.addTypeMapping({ | |
| originalName: 'BaseEntity', | |
| sourceFile: commonFile, | |
| aliasName: 'CommonBase', | |
| }) | |
| resolverStore.addTypeMapping({ | |
| originalName: 'BaseEntity', | |
| sourceFile: legacyFile, | |
| aliasName: 'LegacyBase', | |
| }) | |
| // Create main file with interface inheritance | |
| const mainFile = project.createSourceFile( | |
| 'main.ts', | |
| ` | |
| import { BaseEntity as CommonBase } from "./common-types"; | |
| import { BaseEntity as LegacyBase } from "./legacy-types"; | |
| interface ModernUser extends CommonBase { | |
| email: string; | |
| } | |
| interface LegacyUser extends LegacyBase { | |
| username: string; | |
| } | |
| `, | |
| ) | |
| const modernUserInterface = mainFile.getInterfaces()[0]! | |
| const legacyUserInterface = mainFile.getInterfaces()[1]! | |
| // Extract references from both interfaces | |
| const modernReferences = extractor.extractTypeReferences(modernUserInterface) | |
| const legacyReferences = extractor.extractTypeReferences(legacyUserInterface) | |
| expect(modernReferences).toContain(commonQualifiedName) | |
| expect(legacyReferences).toContain(legacyQualifiedName) | |
| }) |
🤖 Prompt for AI Agents
In tests/traverse/type-reference-extractor.test.ts around lines 102 to 175, the
test uses vacuous assertions and never registers alias mappings for the imported
names, so it won't detect resolver regressions; update the test to add alias
mappings in resolverStore for the imports (map "CommonBase" to
commonQualifiedName and "LegacyBase" to legacyQualifiedName, using the correct
source file contexts), then call extractor.extractTypeReferences and assert that
the returned references include the expected qualified names (or at least that
they resolve to the mapped qualifiedName strings) instead of using
expect(...).toBeGreaterThanOrEqual(0); also replace the generic length checks
with explicit checks that the modernReferences contain commonQualifiedName and
legacyReferences contain legacyQualifiedName.
This change introduces a
ResolverStoreclass to manage the mapping betweenoriginal type names, qualified type names, and type aliases. The key features
of this change are:
and the source file path to prevent naming conflicts.
name, and alias name.
test.
The
resolverStoreis used in theDependencyTraversaltests to ensure thattypes are properly resolved and ordered based on their dependencies.