feat: accept source code or path instead of ts-morph SourceFile #3
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactors generateCode to accept a GenerateCodeOptions object (filePath or sourceCode, callerFile, project, exportEverything). Adds src/input-handler.ts to produce a ts-morph SourceFile from those inputs. Updates docs, tests, and utilities to the new options-based API; consolidates TypeBox import and adjusts package.json format script and dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant GC as generateCode
participant IH as input-handler
participant InProj as ts-morph Project (input)
participant OutProj as ts-morph Project (output)
Caller->>GC: generateCode({ filePath | sourceCode, callerFile?, exportEverything? })
GC->>IH: createSourceFileFromInput(options)
IH->>InProj: ensure Project (provided or new)
alt sourceCode path
IH->>InProj: createSourceFile('temp.ts' or virtual path, sourceCode)
IH->>IH: validate relative imports vs callerFile
else filePath path
IH->>IH: resolveFilePath(filePath, callerFile)
IH->>InProj: addSourceFileAtPath(resolvedPath)
end
IH-->>GC: SourceFile (input)
GC->>OutProj: new Project().createSourceFile('output.ts')
GC->>OutProj: add import { Type, type Static } from "@sinclair/typebox"
GC->>OutProj: parse input (imports → enums → type aliases → interfaces → functions)
GC->>OutProj: emit TypeBox types (respecting exportEverything)
GC-->>Caller: return output.ts full text (string)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
package.json (1)
30-30: Prettier cache: good change; consider adding a check script for CI.Add a non-mutating check to fail CI on unformatted code.
"scripts": { - "format": "prettier --cache --write .", + "format": "prettier --cache --write .", + "format:check": "prettier --cache --check .", "typecheck": "tsc --noEmit", "lint": "eslint" },tests/ts-morph/input-handler.test.ts (2)
25-66: Broaden relative-import detection test coverage.Add cases for
export ... fromand dynamicimport()to lock behavior (currently onlyimport ... fromis covered).describe('hasRelativeImports detection', () => { + test('detects export-from with relative path', () => { + const code = `export { x } from './x'` + expect(() => createSourceFileFromInput({ sourceCode: code })).toThrow( + 'Relative imports are not supported when providing code as string' + ) + }) + + test('detects dynamic import with relative path', () => { + const code = `async function load(){ await import('./x') }` + expect(() => createSourceFileFromInput({ sourceCode: code })).toThrow( + 'Relative imports are not supported when providing code as string' + ) + }) })
205-281: Add a test ensuring relative imports resolve when callerFile is provided.Currently we only assert that code creation is allowed; also verify module-specifier resolution context is the caller’s directory.
describe('createSourceFileFromInput function', () => { + test('resolves relative imports using callerFile directory', () => { + const callerDir = join(tempDir, 'caller') + mkdirSync(callerDir, { recursive: true }) + // Create a relative dependency in the caller’s directory + writeFileSync(join(callerDir, 'dep.ts'), 'export const n = 1') + const code = `import { n } from './dep'; export const m = n` + const sourceFile = createSourceFileFromInput({ + sourceCode: code, + callerFile: join(callerDir, 'entry.ts'), + }) + // Sanity checks + expect(sourceFile.getImportDeclarations()[0]?.getModuleSpecifierValue()).toBe('./dep') + }) })src/ts-morph-codegen.ts (3)
19-24: API redesign via destructured options: consider a thin backward-compat wrapperThis is a breaking change for any callers passing a
SourceFile. If you want a smoother migration, add an optional shim that detects aSourceFileand delegates to the new path. Low effort, high DX value.Here’s a minimal wrapper you could place near the new function (outside this hunk) to keep old usages limping along while you deprecate:
// Back-compat shim (deprecated) export async function generateCode( sourceFileOrOptions: any, legacy?: { exportEverything?: boolean } = { exportEverything: false } ): Promise<string> { if (sourceFileOrOptions && typeof sourceFileOrOptions.getFullText === 'function') { // Called with SourceFile (legacy) const sourceFile = sourceFileOrOptions return generateCodeNew({ sourceCode: sourceFile.getFullText(), callerFile: sourceFile.getFilePath(), project: sourceFile.getProject(), exportEverything: legacy.exportEverything ?? false, }) } // Called with new options return generateCodeNew(sourceFileOrOptions) } // Rename the new impl to avoid name clash in this example export const generateCodeNew = async (opts: GenerateCodeOptions): Promise<string> => { /* ... */ }
31-33: Avoid hardcoded output filename; reuse manipulation settings from input projectCreating a brand-new
Projectis fine, but you lose any customizedmanipulationSettingsor formatting alignment with the input project. Also, hardcoding'output.ts'makes it harder to differentiate multi-file generations.Apply this diff to make the output file name configurable and inherit manipulation settings:
export interface GenerateCodeOptions extends InputOptions { exportEverything?: boolean + outputFileName?: string } -export const generateCode = async ({ +export const generateCode = async ({ sourceCode, filePath, exportEverything = false, ...options }: GenerateCodeOptions): Promise<string> => { const sourceFile = createSourceFileFromInput({ sourceCode, filePath, ...options, }) const processedTypes = new Set<string>() - const newSourceFile = new Project().createSourceFile('output.ts', '', { + const outputFileName = options.outputFileName ?? 'output.ts' + const inputProject = sourceFile.getProject() + const newProject = new Project({ + manipulationSettings: inputProject.getCompilerOptions(), // or copy specific manipulation settings if you set them + }) + const newSourceFile = newProject.createSourceFile(outputFileName, '', { overwrite: true, })Note: if you keep a separate project for output (good to avoid polluting the input project), at least align formatting/manipulation settings.
35-45: Per-specifiertypeimport needs TS ≥ 4.5; prefer two imports for broader compatibility
{ type Static }syntax requires TS 4.5+. If consumers might be on older TS, emit two imports instead: value-only forTypeandimport typeforStatic(supported since TS 3.8). Functionality is identical and safer.- newSourceFile.addImportDeclaration({ - moduleSpecifier: '@sinclair/typebox', - namedImports: [ - 'Type', - { - name: 'Static', - isTypeOnly: true, - }, - ], - }) + newSourceFile.addImportDeclaration({ + moduleSpecifier: '@sinclair/typebox', + namedImports: ['Type'], + }) + newSourceFile.addImportDeclaration({ + moduleSpecifier: '@sinclair/typebox', + isTypeOnly: true, + namedImports: ['Static'], + })Confirm your minimum supported TS version. If it’s TS 5.x across the board, your current single-import approach is fine—just document the requirement.
ARCHITECTURE.md (1)
72-73: Hyphenate “full-text” when used as a compound adjectiveVery minor clarity fix.
-9. **Output**: Finally, the full text content of the newly generated `output.ts` source file (which now contains all the TypeBox schemas and static types) is returned as a string. +9. **Output**: Finally, the full-text content of the newly generated `output.ts` source file (which now contains all the TypeBox schemas and static types) is returned as a string.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
ARCHITECTURE.md(3 hunks)package.json(1 hunks)src/input-handler.ts(1 hunks)src/ts-morph-codegen.ts(4 hunks)tests/integration/wikibase/wikibase.ts(1 hunks)tests/ts-morph/input-handler.test.ts(1 hunks)tests/ts-morph/utils.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/ts-morph/input-handler.test.ts (1)
src/input-handler.ts (1)
createSourceFileFromInput(61-90)
tests/ts-morph/utils.ts (1)
src/ts-morph-codegen.ts (1)
generateCode(19-117)
tests/integration/wikibase/wikibase.ts (1)
src/ts-morph-codegen.ts (1)
generateCode(19-117)
src/ts-morph-codegen.ts (1)
src/input-handler.ts (2)
InputOptions(5-10)createSourceFileFromInput(61-90)
🪛 GitHub Check: CodeQL
src/input-handler.ts
[failure] 14-14: Polynomial regular expression used on uncontrolled data
This regular expression that depends on library input may run slow on strings starting with 'import ' and with many repetitions of ' '.
🪛 markdownlint-cli2 (0.17.2)
ARCHITECTURE.md
26-26: Link fragments should be valid
(MD051, link-fragments)
🪛 LanguageTool
ARCHITECTURE.md
[uncategorized] ~72-~72: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... aliases. 9. Output: Finally, the full text content of the newly generated `output....
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~129-~129: There might be a mistake here.
Context: ...either file paths or source code strings 2. Path Resolution: Handles both absolute...
(QB_NEW_EN)
[grammar] ~130-~130: There might be a mistake here.
Context: ...lative file paths with proper validation 3. Relative Import Validation: Prevents r...
(QB_NEW_EN)
[grammar] ~131-~131: There might be a mistake here.
Context: ...nless a callerFile context is provided 4. Project Context Sharing: Supports pass...
(QB_NEW_EN)
[grammar] ~132-~132: There might be a mistake here.
Context: ...es to maintain import resolution context 5. Error Handling: Provides clear error m...
(QB_NEW_EN)
🔇 Additional comments (10)
tests/integration/wikibase/wikibase.ts (1)
11-15: API migration looks correct.Passing
sourceCodepluscallerFilematches the new InputOptions flow;exportEverything: truepreserved. No issues spotted.tests/ts-morph/utils.ts (2)
6-6: Import string aligns with generator’s type-only import.
type Staticmatches the codegen’sisTypeOnly: true. Good.
21-26: Good: exercising new options shape and reusing the caller’s Project.Passing
{ sourceCode, callerFile, project }will let input handling reuse AST context. No blocking issues.src/ts-morph-codegen.ts (4)
15-17: Public options surface: addGenerateCodeOptionsto package exports (if intended)The interface looks good. If this is part of your public API, ensure it’s re-exported from the package entry (e.g.,
src/index.ts) to avoid forcing deep imports.
25-29: Input creation LGTMGood use of
createSourceFileFromInputto normalizefilePathvssourceCodeflows, and forwarding the rest options for validation. Clear and robust.
47-52: Shared printer and parser options look solidGood single
ts.createPrinter()instance per run and clear propagation ofexportEverythinginto parsers.
1-4: Confirm TS path alias configuration and runtime support
- tsconfig.json includes a paths mapping for internal modules:
"paths": { "@daxserver/validation-schema-codegen/*": ["./src/*"], "@test-fixtures/*": ["./tests/*"] }- A search yielded no relative imports of
input-handler,parsers, orutils; all internal imports use the package alias.- To avoid dual module instances at runtime, ensure your bundler (e.g., Rollup, Webpack) and your test runner (e.g., Bun) are configured to resolve the same alias mapping.
Review comment (updated):
Importing from your own package alias (
@daxserver/validation-schema-codegen/...) is safe given thetsconfig.jsonpaths mapping, but you must confirm that your build and test environments also honor this alias. If they don’t, switch these imports to relative paths (e.g.,./input-handler) to prevent duplicate module graphs or resolution surprises.ARCHITECTURE.md (3)
58-59: Doc states “separate import declarations,” but code emits a single consolidated importCurrent implementation adds a single
import { Type, type Static } from "@sinclair/typebox"(or two imports if you adopt the compatibility suggestion). Update the wording to avoid confusion.-2. **Initialization**: A new in-memory `SourceFile` (`output.ts`) is created to build the generated code. Essential imports for TypeBox (`Type`, `Static`) are added as separate import declarations for better compatibility. +2. **Initialization**: A new in-memory `SourceFile` (`output.ts`) is created to build the generated code. A TypeBox import is added (value import for `Type` and type-only import for `Static`).
60-66: Flow still references ImportParser and omits InterfaceParserImplementation no longer constructs an
ImportParser; import-related type ordering is handled viaDependencyCollector+TypeAliasParser. Also,InterfaceParseris instantiated in code but missing from the list here. Align the doc with reality.Suggested text:
- Step 3: “Parser Instantiation: Instances of EnumParser, TypeAliasParser, InterfaceParser, and FunctionDeclarationParser are created…”
- Step 4: Replace Import Processing paragraph with: “Dependencies are resolved and ordered using DependencyCollector, which traverses import declarations and local types to compute a topological order for type aliases.”
110-126: InputOptions section reads wellThe interface and explanations align with the implementation in
src/input-handler.ts. Clear and helpful.
src/input-handler.ts
Outdated
| const hasRelativeImports = (code: string): boolean => { | ||
| const relativeImportRegex = /import\s+.*?from\s+['"]\.[^'"]*['"]/g | ||
| return relativeImportRegex.test(code) | ||
| } |
There was a problem hiding this comment.
Regex can be vulnerable to performance issues and misses other relative-import forms. Replace with a line-anchored, multi-form detector.
The current /import\s+.*?from\s+['"]\.[^'"]*['"]/g risks catastrophic backtracking on crafted inputs and ignores export ... from and dynamic import(). Use a linear, line-anchored pattern without g, or parse with ts-morph.
-const hasRelativeImports = (code: string): boolean => {
- const relativeImportRegex = /import\s+.*?from\s+['"]\.[^'"]*['"]/g
- return relativeImportRegex.test(code)
-}
+const hasRelativeImports = (code: string): boolean => {
+ // Covers:
+ // - import ... from './x'
+ // - export ... from './x'
+ // - dynamic import('./x')
+ // Anchored to line starts; avoids greedy dot and global flag.
+ const re =
+ /^(?:\s*import\s+(?:[^'"]+?\s+from\s*)?['"]\.\.?\/[^'"]+['"]\s*;?|\s*export\s+[^'"]*from\s*['"]\.\.?\/[^'"]+['"]\s*;?|\s*import\(\s*['"]\.\.?\/[^'"]+['"]\s*\))/m
+ return re.test(code)
+}📝 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 hasRelativeImports = (code: string): boolean => { | |
| const relativeImportRegex = /import\s+.*?from\s+['"]\.[^'"]*['"]/g | |
| return relativeImportRegex.test(code) | |
| } | |
| const hasRelativeImports = (code: string): boolean => { | |
| // Covers: | |
| // - import ... from './x' | |
| // - export ... from './x' | |
| // - dynamic import('./x') | |
| // Anchored to line starts; avoids greedy dot and global flag. | |
| const re = | |
| /^(?:\s*import\s+(?:[^'"]+?\s+from\s*)?['"]\.\.?\/[^'"]+['"]\s*;?|\s*export\s+[^'"]*from\s*['"]\.\.?\/[^'"]+['"]\s*;?|\s*import\(\s*['"]\.\.?\/[^'"]+['"]\s*\))/m | |
| return re.test(code) | |
| } |
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 14-14: Polynomial regular expression used on uncontrolled data
This regular expression that depends on library input may run slow on strings starting with 'import ' and with many repetitions of ' '.
🤖 Prompt for AI Agents
In src/input-handler.ts around lines 12 to 15, the current regex risks
catastrophic backtracking and misses other relative-import forms; replace it
with a line-anchored, non-global detector that covers imports, exports and
dynamic imports or use a TypeScript parser. Specifically, remove the /g flag,
anchor the pattern to line starts (use multiline mode) and match the common
forms: import ... from './...', export ... from './...' and import('./...') with
a single linear regex, or better, switch to ts-morph/TypeScript AST to reliably
detect relative module specifiers.
| export const createSourceFileFromInput = (options: InputOptions): SourceFile => { | ||
| validateInputOptions(options) | ||
|
|
||
| const project = options.project || new Project() | ||
| const { filePath, sourceCode, callerFile } = options | ||
|
|
||
| if (sourceCode) { | ||
| // If callerFile is provided, it means this code came from an existing SourceFile | ||
| // and relative imports should be allowed | ||
| if (hasRelativeImports(sourceCode) && !callerFile) { | ||
| throw new Error( | ||
| 'Relative imports are not supported when providing code as string. Only package imports from node_modules are allowed. Relative imports will be implemented in the future.', | ||
| ) | ||
| } | ||
|
|
||
| return project.createSourceFile('temp.ts', sourceCode) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
When sourceCode is provided, use callerFile’s directory for the virtual file path to enable correct relative-resolution—without colliding with an existing file.
Currently we create 'temp.ts' at the project root, which breaks relative import resolution. Also avoid clobbering an existing SourceFile at callerFile if the caller passes the same Project.
- if (sourceCode) {
+ if (sourceCode) {
// If callerFile is provided, it means this code came from an existing SourceFile
// and relative imports should be allowed
if (hasRelativeImports(sourceCode) && !callerFile) {
throw new Error(
'Relative imports are not supported when providing code as string. Only package imports from node_modules are allowed. Relative imports will be implemented in the future.',
)
}
-
- return project.createSourceFile('temp.ts', sourceCode)
+ const virtualPath = callerFile
+ ? resolve(dirname(callerFile), '__virtual__.ts')
+ : 'temp.ts'
+ return project.createSourceFile(virtualPath, sourceCode, { overwrite: true })
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/input-handler.ts around lines 61–77, when sourceCode is provided we
currently create a flat 'temp.ts' at the project root which breaks
relative-import resolution and can clobber an existing SourceFile in the same
Project; instead compute the virtual file path inside the callerFile's directory
(e.g. dirname(callerFile) + a unique virtual filename) so relative imports
resolve against the caller's folder, and ensure you do not reuse a path already
present in the Project (generate a unique name or append a counter/UUID if a
SourceFile at that path exists) before calling project.createSourceFile so you
neither collide with callerFile nor leave the file at project root.
| if (exportEverything) { | ||
| // When exporting everything, maintain original order | ||
| orderedDependencies = dependencyCollector.collectFromImports( | ||
| importDeclarations, | ||
| options.exportEverything, | ||
| exportEverything, | ||
| ) | ||
| dependencyCollector.addLocalTypes(localTypeAliases, sourceFile) | ||
| } else { | ||
| // When not exporting everything, add local types first so filtering can detect their dependencies | ||
| dependencyCollector.addLocalTypes(localTypeAliases, sourceFile) | ||
| orderedDependencies = dependencyCollector.collectFromImports( | ||
| importDeclarations, | ||
| options.exportEverything, | ||
| exportEverything, | ||
| ) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Dependency ordering branches: behavior check
The “export everything” vs “selective export” paths differ in when local types are added/processed, which is intentional. Please confirm tests cover both branches (including mixed local/imported dependencies and shadowed names) so ordering remains deterministic.
You can sanity-scan tests to ensure both paths are exercised:
Also applies to: 88-99
🏁 Script executed:
#!/bin/bash
rg -nP "exportEverything\s*:\s*true|exportEverything\s*:\s*false|generateCode\(\s*{[^}]*exportEverything" -C2Length of output: 713
Add tests for the exportEverything: false code path
It looks like all existing calls to generateCode({…, exportEverything: true }) exercise only the “export everything” branch. We need coverage of the selective-export branch (when exportEverything is false) to verify that local types are correctly ordered before imports, including scenarios with mixed local/imported dependencies and shadowed names.
Please add tests that:
- Invoke
generateCode({ sourceCode: /* … */, exportEverything: false, callerFile: /* … */ }). - Cover combinations of local type aliases and imported types.
- Assert that the resulting dependency order matches the intended sequence (locals first, then imports).
Suggested locations to extend or add tests:
tests/integration/wikibase/wikibase.ts– currently only exercisesexportEverything: true.- A new unit-test file under
tests/unit/ts-morph-codegen/focusing on both branches and edge cases.
| describe('resolveFilePath function', () => { | ||
| test('resolves absolute path that exists', () => { | ||
| writeFileSync(testFilePath, 'export type Test = string') | ||
|
|
||
| const sourceFile = createSourceFileFromInput({ filePath: testFilePath }) | ||
|
|
||
| expect(sourceFile.getFilePath()).toBe(testFilePath as StandardizedFilePath) | ||
| }) | ||
|
|
||
| test('throws error for absolute path that does not exist', () => { | ||
| const nonExistentPath = join(tempDir, 'non-existent.ts') | ||
|
|
||
| expect(() => createSourceFileFromInput({ filePath: nonExistentPath })).toThrow( | ||
| `Absolute path does not exist: ${nonExistentPath}` | ||
| ) | ||
| }) | ||
|
|
||
| test('resolves relative path from current working directory', () => { | ||
| const relativePath = 'temp-test-input-handler/test.ts' | ||
| writeFileSync(testFilePath, 'export type Test = string') | ||
|
|
||
| const sourceFile = createSourceFileFromInput({ filePath: relativePath }) | ||
|
|
||
| expect(sourceFile.getFilePath()).toBe(testFilePath as StandardizedFilePath) | ||
| }) | ||
|
|
||
| test('resolves relative path from callerFile directory', () => { | ||
| const callerFile = join(tempDir, 'caller.ts') | ||
| const relativeFile = join(tempDir, 'relative.ts') | ||
| writeFileSync(relativeFile, 'export type Test = string') | ||
|
|
||
| const sourceFile = createSourceFileFromInput({ | ||
| filePath: './relative.ts', | ||
| callerFile | ||
| }) | ||
|
|
||
| expect(sourceFile.getFilePath()).toBe(relativeFile as StandardizedFilePath) | ||
| }) | ||
|
|
||
| test('throws error when relative path cannot be resolved', () => { | ||
| expect(() => createSourceFileFromInput({ filePath: './non-existent.ts' })).toThrow( | ||
| 'Could not resolve path: ./non-existent.ts' | ||
| ) | ||
| }) | ||
|
|
||
| test('throws error when multiple resolutions found', () => { | ||
| // Create file in current directory | ||
| const currentDirFile = join(process.cwd(), 'duplicate.ts') | ||
| writeFileSync(currentDirFile, 'export type Test = string') | ||
|
|
||
| // Create file in caller directory | ||
| const callerDir = join(tempDir, 'caller') | ||
| mkdirSync(callerDir, { recursive: true }) | ||
| const callerFile = join(callerDir, 'caller.ts') | ||
| const callerDirFile = join(callerDir, 'duplicate.ts') | ||
| writeFileSync(callerDirFile, 'export type Test = string') | ||
|
|
||
| expect(() => createSourceFileFromInput({ | ||
| filePath: './duplicate.ts', | ||
| callerFile | ||
| })).toThrow('Multiple resolutions found for path: ./duplicate.ts') | ||
|
|
||
| // Cleanup | ||
| rmSync(currentDirFile, { force: true }) | ||
| }) | ||
|
|
||
| test('throws error when path is not a file', () => { | ||
| expect(() => createSourceFileFromInput({ filePath: tempDir })).toThrow( | ||
| `Path is not a file: ${tempDir}` | ||
| ) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Edge case: duplicate candidate paths can false-positive “multiple resolutions.”
If dirname(callerFile) === process.cwd(), the same path can be added twice and trigger the “multiple resolutions” error. Add a test to reproduce and dedupe candidates in implementation (see src/input-handler.ts comment).
describe('resolveFilePath function', () => {
+ test('does not report multiple resolutions when callerDir equals CWD', () => {
+ // Create file in CWD and use a callerFile within the same dir
+ const sameDirFile = join(process.cwd(), 'same-dir.ts')
+ writeFileSync(sameDirFile, 'export type T = 1')
+ try {
+ const sourceFile = createSourceFileFromInput({
+ filePath: './same-dir.ts',
+ callerFile: join(process.cwd(), 'caller.ts'),
+ })
+ expect(sourceFile.getFilePath()).toBe(sameDirFile as StandardizedFilePath)
+ } finally {
+ rmSync(sameDirFile, { force: true })
+ }
+ })
})🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Locate the input-handler implementation file
file=$(fd input-handler.ts | head -n1)
echo "Implementation file found: $file"
# Show context around createSourceFileFromInput
echo "----- createSourceFileFromInput definition -----"
rg -nP 'function\s+createSourceFileFromInput\b' -t ts -C5 "$file" || true
# Show context around resolveFilePath
echo "----- resolveFilePath helper -----"
rg -nP 'function\s+resolveFilePath\b' -t ts -C5 "$file" || true
# Search for any deduplication logic (e.g., Set usage or uniq)
echo "----- dedupe logic search -----"
rg -nP '\\bnew\s+Set\\b|\\.filter\\(|\\.uniq\\(' -t ts -C5 "$file" || trueLength of output: 774
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Print the first 200 lines of the input-handler implementation to locate relevant functions
echo "----- Beginning of src/input-handler.ts -----"
sed -n '1,200p' src/input-handler.ts
# Search for createSourceFileFromInput usages to find its definition
echo "----- Searching for createSourceFileFromInput definition -----"
grep -nR "createSourceFileFromInput" -n src/input-handler.ts
# Search for resolveFilePath to view its implementation
echo "----- Searching for resolveFilePath implementation -----"
grep -nR "resolveFilePath" -n src/input-handler.ts
# Look for any explicit deduplication logic (Set, uniq, filter) in this file
echo "----- Checking for deduplication patterns -----"
grep -nE "new +Set|\\.uniq|\\.filter" -n src/input-handler.ts || echo "No dedupe patterns found"Length of output: 3641
Add deduplication in resolveFilePath and include CWD edge-case test
To prevent false-positive “multiple resolutions” when callerFile is in the current working directory, we need to:
• In src/input-handler.ts, dedupe possiblePaths before filtering for existence.
• In tests/ts-morph/input-handler.test.ts, add a test that verifies resolution succeeds (instead of throwing) when callerDir === process.cwd().
Locations and suggested diffs:
- src/input-handler.ts (inside
resolveFilePath):
// After pushing callerDir and CWD paths:
possiblePaths.push(resolve(process.cwd(), input))
+ // Remove duplicate entries to avoid false-positive multiple resolution errors
+ const uniquePaths = Array.from(new Set(possiblePaths))
-
- const existingPaths = possiblePaths.filter((path) => existsSync(path))
+ const existingPaths = uniquePaths.filter((path) => existsSync(path))
if (existingPaths.length === 0) {
throw new Error(`Could not resolve path: ${input}. Tried: ${possiblePaths.join(', ')}`)
}- tests/ts-morph/input-handler.test.ts (inside the
describe('resolveFilePath function', …)block, after existing tests):
test('throws error when path is not a file', () => {
expect(() => createSourceFileFromInput({ filePath: tempDir })).toThrow(
`Path is not a file: ${tempDir}`
)
})
+ test('does not report multiple resolutions when callerDir equals CWD', () => {
+ // Create file in CWD and use a callerFile within the same dir
+ const sameDirFile = join(process.cwd(), 'same-dir.ts')
+ writeFileSync(sameDirFile, 'export type T = 1')
+ try {
+ const sourceFile = createSourceFileFromInput({
+ filePath: './same-dir.ts',
+ callerFile: join(process.cwd(), 'caller.ts'),
+ })
+ expect(sourceFile.getFilePath()).toBe(sameDirFile as StandardizedFilePath)
+ } finally {
+ rmSync(sameDirFile, { force: true })
+ }
+ })📝 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.
| describe('resolveFilePath function', () => { | |
| test('resolves absolute path that exists', () => { | |
| writeFileSync(testFilePath, 'export type Test = string') | |
| const sourceFile = createSourceFileFromInput({ filePath: testFilePath }) | |
| expect(sourceFile.getFilePath()).toBe(testFilePath as StandardizedFilePath) | |
| }) | |
| test('throws error for absolute path that does not exist', () => { | |
| const nonExistentPath = join(tempDir, 'non-existent.ts') | |
| expect(() => createSourceFileFromInput({ filePath: nonExistentPath })).toThrow( | |
| `Absolute path does not exist: ${nonExistentPath}` | |
| ) | |
| }) | |
| test('resolves relative path from current working directory', () => { | |
| const relativePath = 'temp-test-input-handler/test.ts' | |
| writeFileSync(testFilePath, 'export type Test = string') | |
| const sourceFile = createSourceFileFromInput({ filePath: relativePath }) | |
| expect(sourceFile.getFilePath()).toBe(testFilePath as StandardizedFilePath) | |
| }) | |
| test('resolves relative path from callerFile directory', () => { | |
| const callerFile = join(tempDir, 'caller.ts') | |
| const relativeFile = join(tempDir, 'relative.ts') | |
| writeFileSync(relativeFile, 'export type Test = string') | |
| const sourceFile = createSourceFileFromInput({ | |
| filePath: './relative.ts', | |
| callerFile | |
| }) | |
| expect(sourceFile.getFilePath()).toBe(relativeFile as StandardizedFilePath) | |
| }) | |
| test('throws error when relative path cannot be resolved', () => { | |
| expect(() => createSourceFileFromInput({ filePath: './non-existent.ts' })).toThrow( | |
| 'Could not resolve path: ./non-existent.ts' | |
| ) | |
| }) | |
| test('throws error when multiple resolutions found', () => { | |
| // Create file in current directory | |
| const currentDirFile = join(process.cwd(), 'duplicate.ts') | |
| writeFileSync(currentDirFile, 'export type Test = string') | |
| // Create file in caller directory | |
| const callerDir = join(tempDir, 'caller') | |
| mkdirSync(callerDir, { recursive: true }) | |
| const callerFile = join(callerDir, 'caller.ts') | |
| const callerDirFile = join(callerDir, 'duplicate.ts') | |
| writeFileSync(callerDirFile, 'export type Test = string') | |
| expect(() => createSourceFileFromInput({ | |
| filePath: './duplicate.ts', | |
| callerFile | |
| })).toThrow('Multiple resolutions found for path: ./duplicate.ts') | |
| // Cleanup | |
| rmSync(currentDirFile, { force: true }) | |
| }) | |
| test('throws error when path is not a file', () => { | |
| expect(() => createSourceFileFromInput({ filePath: tempDir })).toThrow( | |
| `Path is not a file: ${tempDir}` | |
| ) | |
| }) | |
| }) | |
| test('throws error when path is not a file', () => { | |
| expect(() => createSourceFileFromInput({ filePath: tempDir })).toThrow( | |
| `Path is not a file: ${tempDir}` | |
| ) | |
| }) | |
| test('does not report multiple resolutions when callerDir equals CWD', () => { | |
| // Create file in CWD and use a callerFile within the same dir | |
| const sameDirFile = join(process.cwd(), 'same-dir.ts') | |
| writeFileSync(sameDirFile, 'export type T = 1') | |
| try { | |
| const sourceFile = createSourceFileFromInput({ | |
| filePath: './same-dir.ts', | |
| callerFile: join(process.cwd(), 'caller.ts'), | |
| }) | |
| expect(sourceFile.getFilePath()).toBe(sameDirFile as StandardizedFilePath) | |
| } finally { | |
| rmSync(sameDirFile, { force: true }) | |
| } | |
| }) | |
| }) |
🤖 Prompt for AI Agents
tests/ts-morph/input-handler.test.ts around lines 99-170: the review asks to add
deduplication in resolveFilePath and a CWD edge-case test; update
src/input-handler.ts so resolveFilePath dedupes possiblePaths (e.g., replace
possiblePaths with a Set or Array.from(new Set(possiblePaths))) before checking
fs.existsSync, then update tests/ts-morph/input-handler.test.ts by adding a test
after the existing cases that creates a callerFile whose directory is
process.cwd() (write any needed duplicate file only once), calls
createSourceFileFromInput with that callerFile and a relative path, and asserts
it resolves (does not throw) to the intended file path.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
ARCHITECTURE.md (2)
140-143: Good fix: examples now await the async generateCode.These updates prevent copy-paste surprises returning a Promise instead of a string.
Also applies to: 149-154, 159-161
26-26: TOC anchor typo resolved.“ImportParser Optimizations” anchor now matches the section header.
🧹 Nitpick comments (4)
src/input-handler.ts (3)
12-18: Broaden relative import detection to include export-from and dynamic import().Current logic only examines standard import declarations. It will miss:
export { X } from './x'export * from './x'- dynamic
import('./x')Consider extending detection to export declarations with module specifiers and scanning for dynamic imports. With ts-morph:
sourceFile.getExportDeclarations().some(d => d.getModuleSpecifierValue()?.startsWith('.') === true)- Find
CallExpressionnodes where identifier isimportand the first arg is a string starting with./or../.
39-41: Minor: dedupe “Tried” paths in error message for clarity.You already dedupe for existence checks; mirror that in the message to avoid duplicate entries when callerDir === CWD.
- throw new Error(`Could not resolve path: ${input}. Tried: ${possiblePaths.join(', ')}`) + const tried = Array.from(new Set(possiblePaths)).join(', ') + throw new Error(`Could not resolve path: ${input}. Tried: ${tried}`)
69-71: Project lifecycle note (optional).If callers pass a shared Project, the temp file fix above keeps it clean. If you often create a new Project here, consider whether consumers need compiler options/tsconfig alignment; otherwise, defaults are fine.
ARCHITECTURE.md (1)
68-69: Tiny wording polish (optional).“the full text content of the newly generated …” can be simplified.
-9. **Output**: Finally, the full text content of the newly generated `output.ts` source file (which now contains all the TypeBox schemas and static types) is returned as a string. +9. **Output**: Finally, the full text of the newly generated `output.ts` source file (which now contains all the TypeBox schemas and static types) is returned as a string.
📜 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 (3)
ARCHITECTURE.md(5 hunks)src/input-handler.ts(1 hunks)tests/ts-morph/input-handler.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/ts-morph/input-handler.test.ts
🧰 Additional context used
🪛 LanguageTool
ARCHITECTURE.md
[uncategorized] ~68-~68: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... aliases. 9. Output: Finally, the full text content of the newly generated `output....
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~125-~125: There might be a mistake here.
Context: ...either file paths or source code strings 2. Path Resolution: Handles both absolute...
(QB_NEW_EN)
[grammar] ~126-~126: There might be a mistake here.
Context: ...lative file paths with proper validation 3. Relative Import Validation: Prevents r...
(QB_NEW_EN)
[grammar] ~127-~127: There might be a mistake here.
Context: ...nless a callerFile context is provided 4. Project Context Sharing: Supports pass...
(QB_NEW_EN)
[grammar] ~128-~128: There might be a mistake here.
Context: ...es to maintain import resolution context 5. Error Handling: Provides clear error m...
(QB_NEW_EN)
| if (existingPaths.length > 1) { | ||
| throw new Error( | ||
| `Multiple resolutions found for path: ${input}. '` + | ||
| `Found: ${existingPaths.join(', ')}. ' + | ||
| 'Please provide a more specific path.`, | ||
| ) |
There was a problem hiding this comment.
Fix broken template literal concatenation (syntax error).
The error message construction mixes template strings and quotes; this won’t compile. Use a single template literal.
- if (existingPaths.length > 1) {
- throw new Error(
- `Multiple resolutions found for path: ${input}. '` +
- `Found: ${existingPaths.join(', ')}. ' +
- 'Please provide a more specific path.`,
- )
- }
+ if (existingPaths.length > 1) {
+ throw new Error(
+ `Multiple resolutions found for path: ${input}. Found: ${existingPaths.join(
+ ', ',
+ )}. Please provide a more specific path.`,
+ )
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (existingPaths.length > 1) { | |
| throw new Error( | |
| `Multiple resolutions found for path: ${input}. '` + | |
| `Found: ${existingPaths.join(', ')}. ' + | |
| 'Please provide a more specific path.`, | |
| ) | |
| if (existingPaths.length > 1) { | |
| throw new Error( | |
| `Multiple resolutions found for path: ${input}. Found: ${existingPaths.join( | |
| ', ', | |
| )}. Please provide a more specific path.`, | |
| ) | |
| } |
🤖 Prompt for AI Agents
In src/input-handler.ts around lines 43 to 48, the error message construction
mixes template literals and string concatenation incorrectly causing a syntax
error; replace the broken concatenation with a single template literal that
includes input and existingPaths.join(', ') so the message reads as one
interpolated string (e.g., use a single backtick-wrapped template literal
combining the descriptive text and the joined paths) and remove the stray quote
characters and plus operators.
| const sourceFile = project.createSourceFile('temp.ts', sourceCode) | ||
|
|
||
| if (!callerFile && hasRelativeImports(sourceFile)) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t leak a “temp” SourceFile; make probing idempotent and collision-free.
Two issues:
- Line 76: creating 'temp.ts' without
{ overwrite: true }will throw on repeated calls with a shared Project. - Lines 86–89: you create a second SourceFile and keep the first in the Project, polluting it and risking name collisions across invocations.
Pattern: create a short-lived probe file to inspect imports, then delete it, then create a single virtual file at a non-colliding path.
- const sourceFile = project.createSourceFile('temp.ts', sourceCode)
+ // Create a temporary file only to inspect imports, then remove it.
+ const probe = project.createSourceFile('__probe__.ts', sourceCode, { overwrite: true })
- if (!callerFile && hasRelativeImports(sourceFile)) {
+ if (!callerFile && hasRelativeImports(probe)) {
throw new Error(
'Relative imports are not supported when providing code as string. ' +
'Only package imports from node_modules are allowed. ' +
'Relative imports will be implemented in the future.',
)
}
- const virtualPath = callerFile ? resolve(dirname(callerFile), '__virtual__.ts') : 'temp.ts'
-
- return project.createSourceFile(virtualPath, sourceCode, { overwrite: true })
+ // Remove probe to keep the Project clean and support repeated calls.
+ probe.delete()
+
+ // Choose a virtual path near the caller to support relative resolution,
+ // and avoid collisions if a file with that name is already in the Project.
+ const base = callerFile ? resolve(dirname(callerFile), '__virtual__.ts') : 'temp.ts'
+ let virtualPath = base
+ let i = 1
+ while (project.getSourceFile(virtualPath)) {
+ virtualPath = base.replace(/\.ts$/, `.${i++}.ts`)
+ }
+ return project.createSourceFile(virtualPath, sourceCode, { overwrite: true })If you prefer, we can extract the “pick non-colliding virtual path” into a small helper placed above for reuse.
Also applies to: 86-89
🤖 Prompt for AI Agents
In src/input-handler.ts around lines 76–78 and 86–89, the current probing
creates a 'temp.ts' SourceFile without overwrite and later creates a second
SourceFile, leaking files into the shared Project and causing collisions on
repeated calls; change probing to (1) pick a non-colliding virtual path (e.g.,
helper getNonCollidingVirtualPath that appends a counter/uuid to a base name or
checks project.getSourceFile) and use that single path for the probe, (2) create
the SourceFile with a unique name (or `{ overwrite: true }` if intentionally
reusing a fixed probe name), (3) perform import probing against that single
file, and (4) immediately remove the probe from the project
(project.removeSourceFile or equivalent) so the Project is not polluted and the
operation is idempotent.
No description provided.