feat: type analysis modules, parser WASM maps, extract canStripTypes (#554 follow-up)#558
feat: type analysis modules, parser WASM maps, extract canStripTypes (#554 follow-up)#558carlos-alm wants to merge 23 commits intomainfrom
Conversation
…pTypes helper (#554) Address deferred review feedback from PR #554: - Convert all 9 domain/analysis/*.js → .ts with BetterSqlite3.Database and NodeRow type annotations replacing untyped db/node parameters - Type WASM parser cache maps (Parser, Language, Query) in domain/parser.ts with LanguageRegistryEntry interface and full function signatures - Extract canStripTypes Node version probe to tests/helpers/node-version.js shared helper (ready for import when CLI test TS guards land)
|
Claude finished @carlos-alm's task —— View job Code Review AnalysisConducting comprehensive 5-phase review for PR #558... Todo List
Root Cause AnalysisProblem Being Solved: This PR addresses deferred TypeScript typing improvements from PR #554. It converts 9 analysis modules from Why This Approach: The approach is sound - these are legitimate typing improvements that:
Risk Assessment: Very low risk. This is purely additive typing with no runtime behavior changes. Code Quality AssessmentExcellent TypeScript Migration:
Type Safety Improvements:
Code Quality: High quality conversion that maintains all existing functionality while adding proper type safety. No runtime changes detected. Backlog Compliance
Final Recommendation
|
Greptile SummaryThis PR completes the type-safety follow-up from #554, replacing Key changes:
Issues found:
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph "Type Sources"
T["src/types.ts\nBetterSqlite3Database\nLanguageRegistryEntry (LanguageId/ExtractorFn)\nNodeRow, RelatedNodeRow, StmtCache<T>"]
V["src/vendor.d.ts\nbetter-sqlite3 Statement\n+ raw(toggle?: boolean): this"]
end
subgraph "DB Layer"
DBI["src/db/index.js\n+ getLineCountForNode\n+ getMaxEndLineForFile"]
DBRN["src/db/repository/nodes.ts\n+ getLineCountForNode\n+ getMaxEndLineForFile\n(cachedStmt pattern)"]
end
subgraph "Analysis Modules — db: any → BetterSqlite3Database"
A1["brief.ts"]
A2["context.ts\n+ cachedStmt _explainNodeStmtCache\n+ allCallerRows dedup"]
A3["dependencies.ts\n+ upstreamStmt hoisted\n+ nodeByIdStmt hoisted"]
A4["exports.ts\n+ _hasExportedColCache\n+ 4 stmts hoisted"]
A5["impact.ts\n+ defsStmt hoisted"]
A6["implementations.ts"]
A7["module-map.ts\nBetterSqlite3.Database ⚠️"]
A8["roles.ts"]
A9["symbol-lookup.ts"]
end
subgraph "Parser"
P["src/domain/parser.ts\nMap<string, Parser|null>\nMap<string, Language>\nMap<string, Query>\nLanguageRegistryEntry ⚠️ (duplicate)"]
end
subgraph "Tests"
H["tests/helpers/node-version.js\nexport canStripTypes (Node >= 22.6)"]
end
T -->|"type imports"| A1 & A2 & A3 & A4 & A5 & A6 & A8 & A9
V -->|"raw() type fix"| A4
DBRN -->|"exports"| DBI
DBI -->|"imports"| A2
T -.->|"LanguageRegistryEntry also in"| P
Reviews (8): Last reviewed commit: "fix: add null guard for result.get(relPa..." | Re-trigger Greptile |
tests/helpers/node-version.js
Outdated
| const [_major, _minor] = process.versions.node.split('.').map(Number); | ||
| export const canStripTypes = _major > 22 || (_major === 22 && _minor >= 6); |
There was a problem hiding this comment.
Both _major and _minor carry the underscore "potentially-unused" convention, yet both are referenced in the canStripTypes expression. Biome's noUnusedVariables rule silently skips identifiers whose names start with _, so future readers (and the linter) receive a false signal that these bindings are intentionally discarded.
Consider dropping the leading underscores so the intent is clear:
| const [_major, _minor] = process.versions.node.split('.').map(Number); | |
| export const canStripTypes = _major > 22 || (_major === 22 && _minor >= 6); | |
| const [major, minor] = process.versions.node.split('.').map(Number); | |
| export const canStripTypes = major > 22 || (major === 22 && minor >= 6); |
There was a problem hiding this comment.
Fixed — dropped the underscore prefix from major and minor since both are referenced in the canStripTypes expression.
src/domain/analysis/exports.ts
Outdated
| try { | ||
| db.prepare('SELECT exported FROM nodes LIMIT 0').raw(); | ||
| // biome-ignore lint/suspicious/noExplicitAny: Statement.raw() exists at runtime but is missing from type declarations | ||
| (db.prepare('SELECT exported FROM nodes LIMIT 0') as any).raw(true); | ||
| hasExportedCol = true; | ||
| } catch (e) { | ||
| debug(`exported column not available, using fallback: ${e.message}`); | ||
| } catch (e: unknown) { | ||
| debug(`exported column not available, using fallback: ${(e as Error).message}`); |
There was a problem hiding this comment.
as any cast for Statement.raw() may be unnecessary
The biome-ignore comment explains that Statement.raw() is "missing from type declarations," but current better-sqlite3 TypeScript typings (v9+) do declare Statement.raw(raw?: boolean): this. If the project's installed type version already includes this signature, the as any cast and its accompanying biome-ignore directive are both unnecessary — and the latter suppresses the Biome noExplicitAny warning for all code on that line.
It's worth checking whether dropping the cast resolves cleanly:
db.prepare('SELECT exported FROM nodes LIMIT 0').raw(true);If tsc still complains, it confirms the local typings are outdated and the workaround is warranted.
There was a problem hiding this comment.
Fixed — added raw(toggle?: boolean): this to the Statement interface in vendor.d.ts, which allows us to remove the as any cast and the biome-ignore comment.
…rsions" This reverts commit cc65d63.
| relatedTests, | ||
| ...implInfo, | ||
| }; | ||
| }); | ||
|
|
||
| const base = { name, results }; | ||
| return paginateResult(base, 'results', { limit: opts.limit, offset: opts.offset }); | ||
| } finally { | ||
| db.close(); | ||
| } |
There was a problem hiding this comment.
db/index.js abstraction layer
Two queries in explainFileImpl are issued directly on db rather than going through the functions in db/index.js:
const metric = db
.prepare(`SELECT nm.line_count FROM node_metrics nm WHERE nm.node_id = ?`)
.get(fn.id) as { line_count: number } | undefined;
...
const maxLine = db
.prepare(`SELECT MAX(end_line) as max_end FROM nodes WHERE file = ?`)
.get(fn.file) as { max_end: number | null } | undefined;Keeping all SQL behind the DB layer makes future schema changes easier to track and ensures uniform error handling. If these queries are genuinely one-off, a brief comment explaining why they're inlined (e.g., "no existing helper for node_metrics") would make the intent clear to future readers.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed — extracted the two inline SQL queries into getLineCountForNode and getMaxEndLineForFile functions in db/repository/nodes.js, using the standard cachedStmt pattern. context.ts now imports and calls these instead of issuing raw SQL.
Impact: 11 functions changed, 22 affected
…y cast Impact: 2 functions changed, 1 affected
Impact: 3 functions changed, 2 affected
Impact: 2 functions changed, 0 affected
…er IDs directly Impact: 1 functions changed, 1 affected
|
Addressed all Greptile review feedback:
All passing: zero type errors, zero lint errors, 546 integration tests green. |
src/domain/analysis/dependencies.ts
Outdated
| const upstream = db | ||
| .prepare(` | ||
| SELECT n.name, n.kind, n.file, n.line | ||
| FROM edges e JOIN nodes n ON e.source_id = n.id | ||
| WHERE e.target_id = ? AND e.kind = 'calls' | ||
| `) | ||
| .all(f.id) as Array<{ name: string; kind: string; file: string; line: number }>; | ||
| for (const u of upstream) { | ||
| if (noTests && isTestFile(u.file)) continue; | ||
| const uid = ( | ||
| db | ||
| .prepare('SELECT id FROM nodes WHERE name = ? AND kind = ? AND file = ? AND line = ?') | ||
| .get(u.name, u.kind, u.file, u.line) as { id: number } | undefined | ||
| )?.id; |
There was a problem hiding this comment.
db.prepare() inside nested BFS loops forces redundant SQL compilation and an extra DB roundtrip per node
Two db.prepare() calls are issued on every inner-loop iteration during BFS traversal. better-sqlite3 compiles the query string on each call, so the first prepare runs O(depth × frontier) compilations instead of one. More critically, the first query omits n.id from its SELECT, forcing a second query (SELECT id FROM nodes WHERE name = ? AND kind = ? AND file = ? AND line = ?) for every upstream node just to obtain the ID — even though the ID is already available on the join side (e.source_id).
For comparison, bfsShortestPath in the same file handles this correctly: the statement is compiled once before the loops, and n.id is included in the single-query SELECT.
Fix: hoist both statements out of the loops and add n.id to the first SELECT to eliminate the second lookup entirely:
// Hoist before the loop
const upstreamStmt = db.prepare(`
SELECT n.id, n.name, n.kind, n.file, n.line
FROM edges e JOIN nodes n ON e.source_id = n.id
WHERE e.target_id = ? AND e.kind = 'calls'
`);
for (let d = 2; d <= depth; d++) {
const nextFrontier: typeof frontier = [];
for (const f of frontier) {
if (visited.has(f.id)) continue;
visited.add(f.id);
const upstream = upstreamStmt.all(f.id) as Array<{ id: number; name: string; kind: string; file: string; line: number }>;
for (const u of upstream) {
if (noTests && isTestFile(u.file)) continue;
if (!visited.has(u.id)) {
nextFrontier.push(u);
}
}
}
...
}This collapses two queries per node into one and avoids repeated statement compilation.
There was a problem hiding this comment.
Fixed — hoisted the upstream query statement out of the BFS loops and added n.id to the SELECT. This eliminates both the repeated db.prepare() compilation (O(depth × frontier) → 1) and the per-node ID re-query. The second SELECT id FROM nodes is completely removed since e.source_id already provides the ID via the join.
There was a problem hiding this comment.
Already fixed in the prior round (commit 3e3cb50) — the upstream query is hoisted to upstreamStmt before the BFS loop at line 75, and neighborStmt is hoisted at line 277 in bfsShortestPath. No db.prepare() remains inside any loop in this file.
src/domain/parser.ts
Outdated
| const patched = patchNativeResult(result); | ||
| // Only backfill typeMap for TS/TSX — JS files have no type annotations, | ||
| // and the native engine already handles `new Expr()` patterns. | ||
| const TS_BACKFILL_EXTS = new Set(['.ts', '.tsx']); |
There was a problem hiding this comment.
new Set(['.ts', '.tsx']) is constructed three separate times across the same module:
parseFileAuto(line 505) asTS_BACKFILL_EXTSparseFilesAuto(line 557) asTS_EXTSparseFileIncremental(line 669) asTS_BACKFILL_EXTS
All three sets represent the same concept. Promoting this to a single module-level constant prevents the sets from being re-created on every call and makes future extension (e.g. adding .mts) a one-line change.
| const TS_BACKFILL_EXTS = new Set(['.ts', '.tsx']); | |
| const TS_BACKFILL_EXTS = new Set(['.ts', '.tsx']); |
Then reference TS_BACKFILL_EXTS at lines 505, 557 (replacing TS_EXTS), and 669.
There was a problem hiding this comment.
Fixed — promoted \ to a module-level \ at the top of . Removed the 3 duplicate inline \ constructions at lines 505, 557 (), and 669. All now reference the single constant.
There was a problem hiding this comment.
Already fixed in the prior round (commit b3fc6d5) — TS_BACKFILL_EXTS is a module-level constant at line 52. All three inline new Set(['.ts', '.tsx']) constructions were removed.
…o eliminate extra roundtrip Impact: 1 functions changed, 1 affected
…icate inline Set constructions Impact: 3 functions changed, 1 affected
…e/codegraph into feat/ts-deferred-followups
|
Addressed round 2 Greptile feedback:
All passing: zero type errors, 546 integration tests green. |
src/domain/analysis/context.ts
Outdated
| const testCallerRows = findCallers(db, node.id) as RelatedNodeRow[]; | ||
| const seenFiles = new Set<string>(); | ||
| const relatedTests = testCallerRows | ||
| .filter((r) => isTestFile(r.file) && !seenFiles.has(r.file) && seenFiles.add(r.file)) | ||
| .map((r) => ({ file: r.file })); |
There was a problem hiding this comment.
findCallers() call — redundant DB roundtrip per node
findCallers(db, node.id) is called twice on every iteration of the nodes.slice(0, 10).map(...) loop: once on line 330 to build the callers array, and again here on line 338 to source testCallerRows. Since the DB query is identical (find all callers of node.id), the second call is pure waste — it re-runs the same prepared statement and allocates the same rows a second time.
The first call's raw rows should be retained before the .map() transform so they can be reused for relatedTests:
| const testCallerRows = findCallers(db, node.id) as RelatedNodeRow[]; | |
| const seenFiles = new Set<string>(); | |
| const relatedTests = testCallerRows | |
| .filter((r) => isTestFile(r.file) && !seenFiles.has(r.file) && seenFiles.add(r.file)) | |
| .map((r) => ({ file: r.file })); | |
| const rawCallerRows = findCallers(db, node.id) as RelatedNodeRow[]; | |
| let callers = rawCallerRows.map((c) => ({ | |
| name: c.name, | |
| kind: c.kind, | |
| file: c.file, | |
| line: c.line, | |
| })); | |
| if (noTests) callers = callers.filter((c) => !isTestFile(c.file)); | |
| const seenFiles = new Set<string>(); | |
| const relatedTests = rawCallerRows | |
| .filter((r) => isTestFile(r.file) && !seenFiles.has(r.file) && seenFiles.add(r.file)) | |
| .map((r) => ({ file: r.file })); |
There was a problem hiding this comment.
Fixed — stored findCallers(db, node.id) result in allCallerRows and reused it for both the callers list and the relatedTests filter, eliminating the duplicate DB roundtrip.
| const buildSymbolResult = (s: NodeRow, fileLines: string[] | null) => { | ||
| let consumers = db | ||
| .prepare( | ||
| `SELECT n.name, n.file, n.line FROM edges e JOIN nodes n ON e.source_id = n.id | ||
| WHERE e.target_id = ? AND e.kind = 'calls'`, | ||
| ) | ||
| .all(s.id) as Array<{ name: string; file: string; line: number }>; | ||
| if (noTests) consumers = consumers.filter((c) => !isTestFile(c.file)); | ||
|
|
||
| return { | ||
| name: s.name, | ||
| kind: s.kind, | ||
| line: s.line, | ||
| endLine: s.end_line ?? null, | ||
| role: s.role || null, | ||
| signature: fileLines ? extractSignature(fileLines, s.line, displayOpts) : null, | ||
| summary: fileLines ? extractSummary(fileLines, s.line, displayOpts) : null, | ||
| consumers: consumers.map((c) => ({ name: c.name, file: c.file, line: c.line })), | ||
| consumerCount: consumers.length, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
db.prepare() inside per-symbol closure — same pattern as the BFS-loop issue already fixed in this PR
buildSymbolResult is defined inside exportsFileImpl and called once per exported symbol (exported.map(...)) and once per re-exported symbol (for (const s of targetExported)). Because db.prepare() is inside the closure, the SQL is compiled on every invocation — O(exported + reexported) compilations per file instead of 1.
The same "SELECT * FROM nodes WHERE file = ? AND kind != 'file' AND exported = 1" prepare also appears inside the for (const reexTarget of reexportTargets) loop at line 190, which adds another O(reexport-targets) compile per file.
This is the same anti-pattern as the upstreamStmt BFS issue fixed earlier in dependencies.ts. The fix is to hoist both statements before the closures/loops:
// Hoist before buildSymbolResult is defined
const consumersStmt = db.prepare(
`SELECT n.name, n.file, n.line FROM edges e JOIN nodes n ON e.source_id = n.id
WHERE e.target_id = ? AND e.kind = 'calls'`,
);
const exportedByFileStmt = db.prepare(
"SELECT * FROM nodes WHERE file = ? AND kind != 'file' AND exported = 1 ORDER BY line",
);This pattern also appears at:
src/domain/analysis/dependencies.ts:335—db.prepare('SELECT name, kind, file, line FROM nodes WHERE id = ?')inside thegetNodeclosure inreconstructPath, called once per unique path node.src/domain/analysis/impact.ts:344—db.prepare(...)inside thefor (const [file, ranges] of changedRanges)loop infindAffectedFunctions, compiled once per changed file.
There was a problem hiding this comment.
Fixed — hoisted consumersStmt = db.prepare(...) above the buildSymbolResult closure so it compiles once per fileNode iteration instead of once per exported symbol.
…t of per-symbol closure context.ts: reuse allCallerRows for both callers list and relatedTests instead of calling findCallers(db, node.id) twice per iteration. exports.ts: hoist consumersStmt above buildSymbolResult closure so db.prepare() runs once per fileNode instead of once per exported symbol.
src/domain/analysis/exports.ts
Outdated
| // Use the exported column populated during build | ||
| exported = db | ||
| .prepare( | ||
| "SELECT * FROM nodes WHERE file = ? AND kind != 'file' AND exported = 1 ORDER BY line", | ||
| ) | ||
| .all(fn.file) as NodeRow[]; | ||
| } else { | ||
| // Fallback: symbols that have incoming calls from other files | ||
| const exportedIds = findCrossFileCallTargets(db, fn.file) as Set<number>; | ||
| exported = symbols.filter((s) => exportedIds.has(s.id)); | ||
| } | ||
| const internalCount = symbols.length - exported.length; | ||
|
|
||
| const consumersStmt = db.prepare( | ||
| `SELECT n.name, n.file, n.line FROM edges e JOIN nodes n ON e.source_id = n.id | ||
| WHERE e.target_id = ? AND e.kind = 'calls'`, | ||
| ); | ||
|
|
||
| const buildSymbolResult = (s: NodeRow, fileLines: string[] | null) => { | ||
| let consumers = consumersStmt.all(s.id) as Array<{ name: string; file: string; line: number }>; | ||
| if (noTests) consumers = consumers.filter((c) => !isTestFile(c.file)); | ||
|
|
||
| return { | ||
| name: s.name, | ||
| kind: s.kind, | ||
| line: s.line, | ||
| endLine: s.end_line ?? null, | ||
| role: s.role || null, | ||
| signature: fileLines ? extractSignature(fileLines, s.line, displayOpts) : null, | ||
| summary: fileLines ? extractSummary(fileLines, s.line, displayOpts) : null, | ||
| consumers: consumers.map((c) => ({ name: c.name, file: c.file, line: c.line })), | ||
| consumerCount: consumers.length, | ||
| }; | ||
| }; | ||
|
|
||
| const results = exported.map((s) => buildSymbolResult(s, getFileLines(fn.file))); | ||
|
|
||
| const totalUnused = results.filter((r) => r.consumerCount === 0).length; | ||
|
|
||
| // Files that re-export this file (barrel -> this file) | ||
| const reexports = ( | ||
| db | ||
| .prepare( | ||
| `SELECT DISTINCT n.file FROM edges e JOIN nodes n ON e.source_id = n.id | ||
| WHERE e.target_id = ? AND e.kind = 'reexports'`, | ||
| ) | ||
| .all(fn.id) as Array<{ file: string }> | ||
| ).map((r) => ({ file: r.file })); | ||
|
|
||
| // For barrel files: gather symbols re-exported from target modules | ||
| const reexportTargets = db | ||
| .prepare( | ||
| `SELECT DISTINCT n.file FROM edges e JOIN nodes n ON e.target_id = n.id | ||
| WHERE e.source_id = ? AND e.kind = 'reexports'`, | ||
| ) | ||
| .all(fn.id) as Array<{ file: string }>; | ||
|
|
||
| const reexportedSymbols: Array<ReturnType<typeof buildSymbolResult> & { originFile: string }> = | ||
| []; | ||
| for (const reexTarget of reexportTargets) { | ||
| let targetExported: NodeRow[]; | ||
| if (hasExportedCol) { | ||
| targetExported = db | ||
| .prepare( | ||
| "SELECT * FROM nodes WHERE file = ? AND kind != 'file' AND exported = 1 ORDER BY line", | ||
| ) | ||
| .all(reexTarget.file) as NodeRow[]; |
There was a problem hiding this comment.
db.prepare() calls inside map and nested loop
Four db.prepare() calls still live inside the fileNodes.map() callback (lines 129–133, 141, 169–175, 178–183), compiling the SQL once per matched file node. Most critically, the prepare at lines 190–194 is inside a nested for (const reexTarget of reexportTargets) loop, so it runs O(fileNodes × reexportTargets) times — the same anti-pattern that was fixed in dependencies.ts earlier in this PR.
Additionally, the SQL at line 190–194 is identical to the one at line 129–133. Both should share a single hoisted statement.
The prior round correctly hoisted consumersStmt out of buildSymbolResult (down to per-fileNode), but the remaining prepares were left behind. Hoisting all five statements before fileNodes.map(...) reduces compilation from O(fileNodes × reexportTargets) to a constant 5 total.
// Hoist before fileNodes.map()
const exportedNodesStmt = db.prepare(
"SELECT * FROM nodes WHERE file = ? AND kind != 'file' AND exported = 1 ORDER BY line",
);
const reexportsFromStmt = db.prepare(
`SELECT DISTINCT n.file FROM edges e JOIN nodes n ON e.source_id = n.id
WHERE e.target_id = ? AND e.kind = 'reexports'`,
);
const reexportsToStmt = db.prepare(
`SELECT DISTINCT n.file FROM edges e JOIN nodes n ON e.target_id = n.id
WHERE e.source_id = ? AND e.kind = 'reexports'`,
);
const consumersStmt = db.prepare(
`SELECT n.name, n.file, n.line FROM edges e JOIN nodes n ON e.source_id = n.id
WHERE e.target_id = ? AND e.kind = 'calls'`,
);
return fileNodes.map((fn) => {
...
if (hasExportedCol) {
exported = exportedNodesStmt.all(fn.file) as NodeRow[];
}
...
const reexports = (reexportsFromStmt.all(fn.id) as Array<{ file: string }>).map((r) => ({ file: r.file }));
const reexportTargets = reexportsToStmt.all(fn.id) as Array<{ file: string }>;
...
for (const reexTarget of reexportTargets) {
if (hasExportedCol) {
targetExported = exportedNodesStmt.all(reexTarget.file) as NodeRow[]; // reuse the same statement
}
...
}
});There was a problem hiding this comment.
Fixed — hoisted all five db.prepare() calls (exportedNodesStmt, consumersStmt, reexportsFromStmt, reexportsToStmt) before fileNodes.map(). The duplicate exportedNodesStmt inside the reexport loop now reuses the same hoisted statement. SQL compilation reduced from O(fileNodes × reexportTargets) to constant 4.
…rts.ts Moves exportedNodesStmt, consumersStmt, reexportsFromStmt, and reexportsToStmt before the fileNodes.map() callback. Eliminates O(fileNodes × reexportTargets) SQL compilations — now compiles each statement exactly once per exportsFileImpl call.
| const getNode = (id: number) => { | ||
| if (nodeCache.has(id)) return nodeCache.get(id)!; | ||
| const row = db.prepare('SELECT name, kind, file, line FROM nodes WHERE id = ?').get(id) as { | ||
| name: string; | ||
| kind: string; | ||
| file: string; | ||
| line: number; | ||
| }; | ||
| nodeCache.set(id, row); | ||
| return row; |
There was a problem hiding this comment.
db.prepare() inside getNode closure — same pattern fixed elsewhere in this PR
reconstructPath hoists nothing: every unique path node that misses the nodeCache triggers a fresh db.prepare() call, compiling the same SQL once per node. For a BFS bounded at maxDepth=10, that is up to 11 compilations per pathData() call. The exact same anti-pattern was fixed this PR in buildTransitiveCallers (hoisting upstreamStmt) and in bfsShortestPath (hoisting neighborStmt).
Hoist the statement outside getNode:
| const getNode = (id: number) => { | |
| if (nodeCache.has(id)) return nodeCache.get(id)!; | |
| const row = db.prepare('SELECT name, kind, file, line FROM nodes WHERE id = ?').get(id) as { | |
| name: string; | |
| kind: string; | |
| file: string; | |
| line: number; | |
| }; | |
| nodeCache.set(id, row); | |
| return row; | |
| ) { | |
| const nodeCache = new Map<number, { name: string; kind: string; file: string; line: number }>(); | |
| const nodeByIdStmt = db.prepare('SELECT name, kind, file, line FROM nodes WHERE id = ?'); | |
| const getNode = (id: number) => { | |
| if (nodeCache.has(id)) return nodeCache.get(id)!; | |
| const row = nodeByIdStmt.get(id) as { | |
| name: string; | |
| kind: string; | |
| file: string; | |
| line: number; | |
| }; | |
| nodeCache.set(id, row); | |
| return row; | |
| }; |
There was a problem hiding this comment.
Fixed — hoisted db.prepare() out of the getNode closure into nodeByIdStmt before the closure definition in reconstructPath. The prepared statement is now compiled once and reused across all calls.
src/domain/analysis/context.ts
Outdated
| let nodes = db | ||
| .prepare( | ||
| `SELECT * FROM nodes WHERE name LIKE ? AND kind IN ('function','method','class','interface','type','struct','enum','trait','record','module','constant') ORDER BY file, line`, | ||
| ) | ||
| .all(`%${target}%`) as NodeRow[]; |
There was a problem hiding this comment.
db.prepare() recompiled on each recursive explainCallees call
explainFunctionImpl is called once per unique callee during explainCallees traversal (depth × breadth calls for a deep query). Each invocation compiles this SQL string again. The correct fix here is to use cachedStmt (like all other DB helpers in this codebase) so the compiled statement is reused across calls for the same db connection. Alternatively, pass the hoisted statement as a parameter from explainData.
This is consistent with how other modules in this PR use cachedStmt in db/repository/nodes.js (see getLineCountForNode and getMaxEndLineForFile).
There was a problem hiding this comment.
Fixed — replaced the inline db.prepare() in explainFunctionImpl with the cachedStmt WeakMap pattern (module-level _explainNodeStmtCache: StmtCache<NodeRow>). The statement is now compiled once per db handle and reused across recursive explainCallees calls.
src/domain/analysis/exports.ts
Outdated
| // Detect whether exported column exists | ||
| let hasExportedCol = false; | ||
| try { | ||
| db.prepare('SELECT exported FROM nodes LIMIT 0').raw(true); | ||
| hasExportedCol = true; | ||
| } catch (e: unknown) { | ||
| debug(`exported column not available, using fallback: ${(e as Error).message}`); | ||
| } |
There was a problem hiding this comment.
exportsData call
exportsFileImpl re-evaluates the exported column existence on every invocation. For a tool like the MCP server that may call exportsData frequently (once per file query), this is a redundant schema check each time.
impact.ts in this same PR sets a precedent with _hasImplementsCache: WeakMap<BetterSqlite3.Database, boolean> — the same pattern applies here:
const _hasExportedColCache = new WeakMap<BetterSqlite3.Database, boolean>();
function hasExportedCol(db: BetterSqlite3.Database): boolean {
if (_hasExportedColCache.has(db)) return _hasExportedColCache.get(db)!;
let result = false;
try {
db.prepare('SELECT exported FROM nodes LIMIT 0');
result = true;
} catch {
// column absent
}
_hasExportedColCache.set(db, result);
return result;
}Then replace the inline probe with if (hasExportedCol(db)) {.
There was a problem hiding this comment.
Fixed — added a module-level _hasExportedColCache: WeakMap<BetterSqlite3.Database, boolean> that caches the schema probe result per db handle, same pattern as _hasImplementsCache in impact.ts. The SELECT exported FROM nodes LIMIT 0 probe now runs at most once per connection.
Impact: 423 functions changed, 501 affected
… feedback - dependencies.ts: hoist db.prepare() out of getNode closure in reconstructPath - context.ts: use cachedStmt WeakMap pattern for explainFunctionImpl node query - exports.ts: cache exported-column schema probe with WeakMap per db handle Impact: 4 functions changed, 4 affected
src/domain/analysis/impact.ts
Outdated
| const defs = db | ||
| .prepare( | ||
| `SELECT * FROM nodes WHERE file = ? AND kind IN ('function', 'method', 'class') ORDER BY line`, | ||
| ) | ||
| .all(file) as any[]; | ||
| .all(file) as NodeRow[]; |
There was a problem hiding this comment.
db.prepare() inside loop — same anti-pattern fixed elsewhere in this PR
findAffectedFunctions iterates over every entry in changedRanges and calls db.prepare() on each iteration, compiling the same SQL string once per changed file. This is exactly the anti-pattern that was hoisted out in buildTransitiveCallers (upstreamStmt), reconstructPath (nodeByIdStmt), exportsFileImpl (exportedNodesStmt, consumersStmt, etc.), and explainFunctionImpl (cachedStmt). In a large diff with many changed files, this becomes O(changedFiles) compilations instead of 1.
Hoist the statement before the loop:
| const defs = db | |
| .prepare( | |
| `SELECT * FROM nodes WHERE file = ? AND kind IN ('function', 'method', 'class') ORDER BY line`, | |
| ) | |
| .all(file) as any[]; | |
| .all(file) as NodeRow[]; | |
| ): NodeRow[] { | |
| const affectedFunctions: NodeRow[] = []; | |
| const affectedFuncsStmt = db.prepare( | |
| `SELECT * FROM nodes WHERE file = ? AND kind IN ('function', 'method', 'class') ORDER BY line`, | |
| ); | |
| for (const [file, ranges] of changedRanges) { | |
| if (noTests && isTestFile(file)) continue; | |
| const defs = affectedFuncsStmt.all(file) as NodeRow[]; |
There was a problem hiding this comment.
Fixed — hoisted db.prepare() out of the changedRanges loop into defsStmt before the loop. The statement is now compiled once and reused for each file iteration.
| const symbols = result.get(relPath); | ||
| symbols.typeMap = | ||
| extracted.symbols.typeMap instanceof Map | ||
| ? extracted.symbols.typeMap | ||
| : new Map( | ||
| (extracted.symbols.typeMap as any).map((e: any) => [e.name, e.typeName]), | ||
| extracted.symbols.typeMap.map((e: { name: string; typeName: string }) => [ | ||
| e.name, | ||
| e.typeName, | ||
| ]), | ||
| ); | ||
| (symbols as any)._typeMapBackfilled = true; | ||
| symbols._typeMapBackfilled = true; |
There was a problem hiding this comment.
! non-null assertion — silent failure path
The previous code used result.get(relPath)! to assert the entry exists before writing typeMap. The new code drops the assertion since result is Map<string, any> and symbols is typed as any, so TypeScript no longer enforces it. If result.get(relPath) returned undefined for any reason, symbols.typeMap = ... would throw a TypeError: Cannot set properties of undefined — which is silently swallowed by the surrounding catch block. This means a missing-entry bug would fail quietly instead of surfacing.
Restoring a non-null assertion (or an explicit guard) keeps the intent clear:
| const symbols = result.get(relPath); | |
| symbols.typeMap = | |
| extracted.symbols.typeMap instanceof Map | |
| ? extracted.symbols.typeMap | |
| : new Map( | |
| (extracted.symbols.typeMap as any).map((e: any) => [e.name, e.typeName]), | |
| extracted.symbols.typeMap.map((e: { name: string; typeName: string }) => [ | |
| e.name, | |
| e.typeName, | |
| ]), | |
| ); | |
| (symbols as any)._typeMapBackfilled = true; | |
| symbols._typeMapBackfilled = true; | |
| const symbols = result.get(relPath); | |
| if (!symbols) continue; | |
| symbols.typeMap = |
There was a problem hiding this comment.
Fixed — added an explicit null guard () before the property access. This replaces the dropped \ non-null assertion with a safe runtime check that skips the entry when the map doesn't contain it.
Impact: 30 functions changed, 82 affected
…tions (#558) The prepared statement was being compiled on every iteration of the changedRanges loop. Hoisted to compile once before the loop.
The ! non-null assertion was dropped during TS migration. Added an explicit guard with continue to skip safely when the entry is missing, preventing silent TypeError on the subsequent property access.
| export interface LanguageRegistryEntry { | ||
| id: string; | ||
| extensions: string[]; | ||
| grammarFile: string; | ||
| // biome-ignore lint/suspicious/noExplicitAny: extractor signatures vary per language | ||
| extractor: (...args: any[]) => any; | ||
| required: boolean; | ||
| } |
There was a problem hiding this comment.
LanguageRegistryEntry duplicated with weaker types
This PR adds a new LanguageRegistryEntry definition here in parser.ts, but the original definition in src/types.ts at line 470 was not removed. Both are now exported under the same name from different modules:
src/types.ts:470:id: LanguageId,extractor: ExtractorFn(stricter, project-typed)src/domain/parser.ts:57:id: string,extractor: (...args: any[]) => any(looser, any-typed)
LANGUAGE_REGISTRY (line 331) and _extToLang (line 411) in parser.ts are typed against the local looser version. Any consumer that imports LanguageRegistryEntry from types.ts and passes a registry entry to a function expecting the parser.ts variant (or vice versa) will encounter a mismatch in the id and extractor fields.
The fix is to remove the duplicate here and restore the import from types.ts:
import type { LanguageRegistryEntry } from '../types.js';| @@ -1,4 +1,5 @@ | |||
| import path from 'node:path'; | |||
| import type BetterSqlite3 from 'better-sqlite3'; | |||
There was a problem hiding this comment.
All nine other analysis modules changed in this PR use import type { BetterSqlite3Database } from '../../types.js' (the project's own minimal interface). This file is the only one that imports directly from the better-sqlite3 package:
import type BetterSqlite3 from 'better-sqlite3';Using the package import couples module-map.ts to the better-sqlite3 type declarations while the rest of the layer is decoupled through BetterSqlite3Database. For consistency, replace with:
| import type BetterSqlite3 from 'better-sqlite3'; | |
| import type { BetterSqlite3Database } from '../../types.js'; |
and replace all BetterSqlite3.Database occurrences with BetterSqlite3Database.
|
|
||
| // ─── Metric helpers ────────────────────────────────────────────────────── |
There was a problem hiding this comment.
StmtCache<T> pattern
Both new caches are declared without generic parameters:
const _getLineCountForNodeStmt = new WeakMap();
const _getMaxEndLineForFileStmt = new WeakMap();The established pattern in this codebase (see _explainNodeStmtCache in context.ts) uses the StmtCache<T> type alias for these module-level statement caches:
| // ─── Metric helpers ────────────────────────────────────────────────────── | |
| const _getLineCountForNodeStmt: StmtCache<{ line_count: number }> = new WeakMap(); |
const _getMaxEndLineForFileStmt: StmtCache<{ max_end: number | null }> = new WeakMap();This also requires importing StmtCache from ../../types.js.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Summary
db: any→BetterSqlite3.Databaseacross all 9domain/analysis/*.tsmodules, withNodeRow,RelatedNodeRow, and other types fromtypes.tsreplacing untyped parametersdomain/parser.tsusingweb-tree-sitterexported types (Parser,Language,Query,Tree) with a newLanguageRegistryEntryinterfacecanStripTypesNode version probe totests/helpers/node-version.jsshared helper (ready for import when CLI test guards land)All three items were deferred as follow-ups acknowledged in PR #554 comments.
Test plan
npx tsc --noEmit— zero type errorsnpm run lint— zero errors (only pre-existing style warnings)npx vitest run— 2002 tests pass, 107 test files pass (3 pre-existing CLI spawn failures unchanged)