feat: track dynamic import() expressions as graph edges#389
Conversation
Extract dynamic import() calls in both query-based and walk-based
extraction paths. Adds 'dynamic-imports' edge kind to CORE_EDGE_KINDS.
- Add extractDynamicImportsWalk() for the query-based fast path
(tree-sitter Query patterns don't match import() function type)
- Add extractDynamicImportNames() to extract destructured names from
patterns like `const { a } = await import('./foo.js')`
- Update builder.js edge kind selection to emit 'dynamic-imports'
edges including barrel resolution propagation
- Add query-walk parity test for dynamic import expressions
Impact: 6 functions changed, 24 affected
|
Claude finished @carlos-alm's task —— View job PR Review: Dynamic Import TrackingI'm reviewing this PR to add dynamic import() expression tracking to codegraph. Let me analyze the changes systematically: Todo List:
Phase 1: Root Cause Analysis ✅Problem Being Solved: Codegraph was reporting false "zero consumers" for symbols that are actually consumed via dynamic Why This Approach: The solution adds a targeted tree walk ( Risk Assessment: Low risk - this is an additive feature that doesn't modify existing extraction logic. The approach is consistent with the existing pattern of using targeted walks for edge cases (like Phase 2: Configuration Change Detection ✅No configuration files modified (biome.json, eslint, tsconfig, package.json unchanged). No linting rules disabled, test timeouts increased, or coverage thresholds lowered. This is purely a feature addition without any rule relaxation. Phase 3: Test Integrity ✅Test Coverage: Added comprehensive test case for dynamic imports in query-walk-parity.test.js covering:
Test Quality: The test properly validates both extraction paths (query vs walk) produce identical results, ensuring the new functionality maintains parity. The normalization function correctly handles the new No Red Flags: No test assertions weakened, no skipped validations, appropriate test coverage for the new feature. Phase 4: Code Quality Review ✅Implementation Quality:
Edge Cases Handled:
Security & Safety:
Edge Kind Integration:
Final AssessmentRoot Cause AnalysisProblem Being Solved: Dynamic imports causing false "zero consumers" reports - a real user pain point Critical ConcernsNone identified. This is a clean, well-implemented feature addition. Final Recommendation
Recommendation: Approved for merge. This PR successfully addresses the dynamic import tracking gap while maintaining code quality and test coverage standards. |
Greptile SummaryThis PR adds dynamic Key changes:
Confidence Score: 5/5
Last reviewed commit: e97e75b |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/extractors/javascript.js
Outdated
| // Extract destructured names from parent context: | ||
| // const { a, b } = await import('./foo.js') | ||
| // import('./foo.js').then(({ a, b }) => ...) |
There was a problem hiding this comment.
Misleading .then() comment — names are never extracted for that pattern
The comment claims that destructured names from import('./foo.js').then(({ a, b }) => ...) are extracted, but extractDynamicImportNames only climbs the AST up to a variable_declarator. For the .then() pattern, the direct parent of the call_expression is a member_expression, not an await_expression or variable_declarator, so extractDynamicImportNames will always return [] for that case.
The file-level dynamic-imports edge is still emitted (no data is lost), but the comment sets a false expectation and could mislead future contributors into thinking the .then() case is already handled.
| // Extract destructured names from parent context: | |
| // const { a, b } = await import('./foo.js') | |
| // import('./foo.js').then(({ a, b }) => ...) | |
| // Extract destructured names from parent context: | |
| // const { a, b } = await import('./foo.js') | |
| // (standalone import('./foo.js').then(...) calls produce an edge with empty names) |
There was a problem hiding this comment.
Already addressed in commit ae0155e — the comment was corrected to say .then(...) calls produce an edge with empty names, matching the actual behavior.
| const strArg = findChild(args, 'string'); | ||
| if (strArg) { | ||
| const modPath = strArg.text.replace(/['"]/g, ''); |
There was a problem hiding this comment.
Template-literal and variable import paths silently skipped
Both extractDynamicImportsWalk (query path) and the walk path in extractSymbolsWalk look for a child node of type string to extract the module path. This means the following patterns produce no edge and no warning:
import(`./plugins/${name}.js`); // template_string node, not string
const p = './utils.js';
const mod = await import(p); // identifier node, not stringThese are admittedly hard to resolve statically, but they account for a meaningful portion of real-world dynamic imports (especially lazy-loaded plugins). Leaving them silently uncovered can lead to the same "false zero-consumers" reports the PR was designed to fix.
A simple improvement would be to log a debug-level warning when an import() call is encountered but the path cannot be statically resolved, so users are at least aware of the gap.
There was a problem hiding this comment.
Already addressed in commit ae0155e — both the walk path (extractDynamicImportsWalk) and the query path (walkJavaScriptNode) already emit a debug() warning when a dynamic import() has a non-static path (template literal or variable).
|
Claude finished @carlos-alm's task —— View job PR Review: Dynamic Import TrackingI'm reviewing this PR to add dynamic import() expression tracking to codegraph. Let me analyze the changes systematically: Todo List:
Phase 1: Root Cause Analysis ✅Problem Being Solved: Codegraph was reporting false "zero consumers" for symbols that are actually consumed via dynamic Why This Approach: The solution adds a targeted tree walk ( Risk Assessment: Low risk - this is an additive feature that doesn't modify existing extraction logic. The approach is consistent with the existing pattern of using targeted walks for edge cases (like Phase 2: Configuration Change Detection ✅No configuration files modified (biome.json, eslint, tsconfig, package.json unchanged). No linting rules disabled, test timeouts increased, or coverage thresholds lowered. This is purely a feature addition without any rule relaxation. Phase 3: Test Integrity ✅Test Coverage: Added comprehensive test case for dynamic imports in query-walk-parity.test.js covering:
Test Quality: The test properly validates both extraction paths (query vs walk) produce identical results, ensuring the new functionality maintains parity. The normalization function correctly handles the new No Red Flags: No test assertions weakened, no skipped validations, appropriate test coverage for the new feature. Phase 4: Code Quality Review ✅Implementation Quality:
Edge Cases Handled:
Security & Safety:
Edge Kind Integration:
Final AssessmentRoot Cause AnalysisProblem Being Solved: Dynamic imports causing false "zero consumers" reports - a real user pain point Critical ConcernsNone identified. This is a clean, well-implemented feature addition. Final Recommendation
Recommendation: Approved for merge. This PR successfully addresses the dynamic import tracking gap while maintaining code quality and test coverage standards. |
|
Addressed both review comments: 1. Misleading .then() comment (ccfd2a7): Fixed the comment to accurately state that .then() pattern produces an edge with empty names, not extracted destructured names. Added backlog ID 82 to track future .then() callback name extraction. 2. Template-literal/variable import paths (ccfd2a7): Added debug-level warning when import() has a non-static path (template literal or variable) so users are aware of the gap. These are logged via debug() — visible with DEBUG=codegraph. Backlog items added (04cebcc): ID 81 (dynamic import tracking — remaining gaps: native engine, re-export consumer counting) and ID 82 (.then() callback name extraction). |
…orts - Fix comment: .then() pattern does not extract destructured names (edge has empty names) - Add debug-level warning when import() has a template literal or variable path that can't be statically resolved Impact: 3 functions changed, 3 affected
Impact: 3 functions changed, 3 affected
carlos-alm
force-pushed
the
feat/dynamic-import-edges
branch
from
04cebcc to
0f50eb3
Compare
|
Claude finished @carlos-alm's task —— View job PR Review: Dynamic Import TrackingI'm reviewing this PR to add dynamic import() expression tracking to codegraph. Let me analyze the changes systematically: Todo List:
Phase 1: Root Cause Analysis ✅Problem Being Solved: Codegraph was reporting false "zero consumers" for symbols that are actually consumed via dynamic Why This Approach: The solution adds a targeted tree walk ( Risk Assessment: Low risk - this is an additive feature that doesn't modify existing extraction logic. The approach is consistent with the existing pattern of using targeted walks for edge cases (like Phase 2: Configuration Change Detection ✅No configuration files modified (biome.json, eslint, tsconfig, package.json unchanged). No linting rules disabled, test timeouts increased, or coverage thresholds lowered. This is purely a feature addition without any rule relaxation. Phase 3: Test Integrity ✅Test Coverage: Added comprehensive test case for dynamic imports in query-walk-parity.test.js covering:
Test Quality: The test properly validates both extraction paths (query vs walk) produce identical results, ensuring the new functionality maintains parity. The normalization function correctly handles the new No Red Flags: No test assertions weakened, no skipped validations, appropriate test coverage for the new feature. Phase 4: Code Quality Review ✅Implementation Quality:
Edge Cases Handled:
Security & Safety:
Edge Kind Integration:
Verified Recent Fixes ✅Commit f89f0ca correctly addressed both review concerns:
Commit 0f50eb3 ✅: Added backlog items ID 81 and 82 to track the remaining gaps Final AssessmentRoot Cause AnalysisProblem Being Solved: Dynamic imports causing false "zero consumers" reports - a real user pain point Critical ConcernsNone identified. This is a clean, well-implemented feature addition. Final Recommendation
Recommendation: Approved for merge. This PR successfully addresses the dynamic import tracking gap while maintaining code quality and test coverage standards. |
| if (nameNode.type === 'array_pattern') { | ||
| const names = []; | ||
| for (let i = 0; i < nameNode.childCount; i++) { | ||
| const child = nameNode.child(i); | ||
| if (child.type === 'identifier') names.push(child.text); | ||
| } | ||
| return names; | ||
| } |
There was a problem hiding this comment.
rest_pattern elements in array destructuring are silently dropped.
The array_pattern loop only pushes children of type identifier, but a rest element like ...rest has type rest_pattern whose single child is the actual identifier. This means:
const [a, ...rest] = await import('./utils.js');
// names → ['a'] — 'rest' is silently droppedAny call like rest.method() would fail to resolve to ./utils.js. The comment notes this case as "rare but possible", making it worth handling completely:
| if (nameNode.type === 'array_pattern') { | |
| const names = []; | |
| for (let i = 0; i < nameNode.childCount; i++) { | |
| const child = nameNode.child(i); | |
| if (child.type === 'identifier') names.push(child.text); | |
| } | |
| return names; | |
| } | |
| if (nameNode.type === 'array_pattern') { | |
| const names = []; | |
| for (let i = 0; i < nameNode.childCount; i++) { | |
| const child = nameNode.child(i); | |
| if (child.type === 'identifier') names.push(child.text); | |
| else if (child.type === 'rest_pattern') { | |
| const inner = child.child(0) || child.childForFieldName('name'); | |
| if (inner && inner.type === 'identifier') names.push(inner.text); | |
| } | |
| } | |
| return names; | |
| } |
There was a problem hiding this comment.
Fixed in e97e75b — the \ loop now handles \ elements by extracting the inner identifier, so \ correctly yields .
| name: 'dynamic import() expressions', | ||
| file: 'test.js', | ||
| code: ` | ||
| const { readFile } = await import('fs/promises'); | ||
| const mod = await import('./utils.js'); | ||
| import('./side-effect.js'); | ||
| `, | ||
| }, |
There was a problem hiding this comment.
Missing coverage for aliased destructuring in parity test.
The new test case covers shorthand destructuring ({ readFile }), namespace binding (mod), and bare side-effect imports, but the pair_pattern branch in extractDynamicImportNames (i.e., { a: localName }) is exercised by neither the query nor the walk path here. Adding one line would close that gap and verify both paths agree on the key-vs-alias choice:
| name: 'dynamic import() expressions', | |
| file: 'test.js', | |
| code: ` | |
| const { readFile } = await import('fs/promises'); | |
| const mod = await import('./utils.js'); | |
| import('./side-effect.js'); | |
| `, | |
| }, | |
| code: ` | |
| const { readFile } = await import('fs/promises'); | |
| const { readFile: rf } = await import('node:fs/promises'); | |
| const mod = await import('./utils.js'); | |
| import('./side-effect.js'); | |
| `, |
There was a problem hiding this comment.
Fixed in e97e75b — added \ to the parity test to exercise the \ branch in both engines.
|
Claude finished @carlos-alm's task —— View job PR Review: Dynamic Import TrackingI'm reviewing this PR to add dynamic import() expression tracking to codegraph. Let me analyze the changes systematically: Todo List:
Phase 1: Root Cause Analysis ✅Problem Being Solved: Codegraph was reporting false "zero consumers" for symbols that are actually consumed via dynamic Why This Approach: The solution adds a targeted tree walk ( Risk Assessment: Low risk - this is an additive feature that doesn't modify existing extraction logic. The approach is consistent with the existing pattern of using targeted walks for edge cases (like Phase 2: Configuration Change Detection ✅No configuration files modified (biome.json, eslint, tsconfig, package.json unchanged). No linting rules disabled, test timeouts increased, or coverage thresholds lowered. This is purely a feature addition without any rule relaxation. Phase 3: Test Integrity ✅Test Coverage: Added comprehensive test case for dynamic imports in query-walk-parity.test.js covering:
Test Quality: The test properly validates both extraction paths (query vs walk) produce identical results, ensuring the new functionality maintains parity. The normalization function correctly handles the new No Red Flags: No test assertions weakened, no skipped validations, appropriate test coverage for the new feature. Phase 4: Code Quality Review ✅Implementation Quality:
Edge Cases Handled:
Security & Safety:
Edge Kind Integration:
Verified Recent Fixes ✅Commit f89f0ca correctly addressed both review concerns:
Commit 0f50eb3 ✅: Added backlog items ID 81 and 82 to track the remaining gaps Final AssessmentRoot Cause AnalysisProblem Being Solved: Dynamic imports causing false "zero consumers" reports - a real user pain point Critical ConcernsNone identified. This is a clean, well-implemented feature addition. Final Recommendation
Recommendation: Approved for merge. This PR successfully addresses the dynamic import tracking gap while maintaining code quality and test coverage standards. |
…t test
Address PR review feedback:
- Extract `...rest` identifiers from array_pattern in extractDynamicImportNames
- Add `{ readFile: rf }` aliased destructuring case to query-walk parity test
Impact: 1 functions changed, 5 affected
|
Claude finished @carlos-alm's task —— View job Claude is working on this task PR Review: Dynamic Import TrackingI'm reviewing this PR to add dynamic import() expression tracking to codegraph. Let me analyze the changes systematically: Todo List:
Phase 1: Root Cause Analysis ✅Problem Being Solved: Codegraph was reporting false "zero consumers" for symbols that are actually consumed via dynamic Why This Approach: The solution adds a targeted tree walk ( Risk Assessment: Low risk - this is an additive feature that doesn't modify existing extraction logic. The approach is consistent with the existing pattern of using targeted walks for edge cases (like Phase 2: Configuration Change Detection ✅No configuration files modified (biome.json, eslint, tsconfig, package.json unchanged). No linting rules disabled, test timeouts increased, or coverage thresholds lowered. This is purely a feature addition without any rule relaxation. Phase 3: Test Integrity ✅Test Coverage: Added comprehensive test case for dynamic imports in query-walk-parity.test.js covering:
Test Quality: The test properly validates both extraction paths (query vs walk) produce identical results, ensuring the new functionality maintains parity. The normalization function correctly handles the new No Red Flags: No test assertions weakened, no skipped validations, appropriate test coverage for the new feature. Phase 4: Code Quality Review ✅Implementation Quality:
Edge Cases Handled:
Security & Safety:
Edge Kind Integration:
Verified Recent Fixes ✅Commit e97e75b correctly addressed both review concerns:
Earlier fixes also addressed:
Final AssessmentRoot Cause AnalysisProblem Being Solved: Dynamic imports causing false "zero consumers" reports - a real user pain point Critical ConcernsNone identified. This is a clean, well-implemented feature addition. Final Recommendation
Recommendation: Approved for merge. This PR successfully addresses the dynamic import tracking gap while maintaining code quality and test coverage standards. |
1 participant
Summary
import()calls in both query-based and walk-based JS/TS extraction pathsdynamic-importsedge kind toCORE_EDGE_KINDSimport()expressions as graph edges, fixing false "zero consumers" reports for symbols consumed via dynamic importsChanges
src/extractors/javascript.js: AddextractDynamicImportsWalk()targeted walk for the query-based fast path (tree-sitter Query patterns can't matchimport()function type). AddextractDynamicImportNames()to extract destructured names from patterns likeconst { a } = await import('./foo.js')src/kinds.js: Add'dynamic-imports'toCORE_EDGE_KINDSsrc/builder.js: Update edge kind selection to emitdynamic-importsedges, including barrel resolution propagationtests/engines/query-walk-parity.test.js: Add parity test for dynamic import expressionsVerification
dynamic-importsedges created when building codegraph's own graphTest plan
npx vitest run tests/engines/query-walk-parity.test.js— 14 tests pass (including new dynamic import case)npx vitest run tests/parsers/javascript.test.js— 20 tests passnpm test— 1564 tests passnode src/cli.js build . --no-incremental --engine wasm— 95 dynamic-imports edges created