refactor: decompose queries.js into src/analysis/ and src/shared/ modules (roadmap 3.4)#425
Conversation
…shared/ modules
queries.js (2289 lines) decomposed into focused modules per roadmap 3.4:
src/shared/normalize.js — normalizeSymbol, kindIcon
src/shared/generators.js — iterListFunctions, iterRoles, iterWhere
src/analysis/symbol-lookup.js — findMatchingNodes, queryNameData, whereData,
listFunctionsData, childrenData
src/analysis/impact.js — impactAnalysisData, fnImpactData, diffImpactData,
diffImpactMermaid
src/analysis/dependencies.js — fileDepsData, fnDepsData, pathData
src/analysis/module-map.js — moduleMapData, statsData
src/analysis/context.js — contextData, explainData
src/analysis/exports.js — exportsData
src/analysis/roles.js — rolesData
queries.js is now a ~36-line barrel re-export for backward compatibility.
All 85 test files pass.
Greptile SummaryThis PR completes roadmap item 3.4 by decomposing the 2289-line Key changes:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
QJ["src/queries.js\n(barrel re-export)"]
subgraph analysis["src/analysis/"]
CTX["context.js\ncontextData, explainData"]
DEP["dependencies.js\nfileDepsData, fnDepsData, pathData"]
EXP["exports.js\nexportsData"]
IMP["impact.js\nimpactAnalysisData, fnImpactData\ndiffImpactData, diffImpactMermaid"]
MM["module-map.js\nmoduleMapData, statsData"]
ROL["roles.js\nrolesData"]
SL["symbol-lookup.js\nfindMatchingNodes, queryNameData\nwhereData, listFunctionsData, childrenData"]
end
subgraph shared["src/shared/"]
FU["file-utils.js\nsafePath, readSourceRange\nextractSummary, extractSignature\ncreateFileLinesReader, isFileLikeTarget"]
GEN["generators.js\niterListFunctions, iterRoles, iterWhere"]
HIE["hierarchy.js\nresolveMethodViaHierarchy"]
NOR["normalize.js\ngetFileHash, kindIcon, normalizeSymbol"]
end
QJ --> CTX & DEP & EXP & IMP & MM & ROL & SL
QJ --> GEN & NOR
CTX --> SL
CTX --> FU & HIE & NOR
DEP --> SL
DEP --> HIE & NOR
EXP --> FU
IMP --> SL & NOR
MM --> NOR
ROL --> NOR
SL --> NOR
Last reviewed commit: 69bea62 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| maxNesting: cRow.max_nesting, | ||
| maintainabilityIndex: cRow.maintainability_index || 0, | ||
| halsteadVolume: cRow.halstead_volume || 0, | ||
| }; | ||
| } | ||
| } catch { | ||
| /* table may not exist */ | ||
| } | ||
|
|
||
| return { | ||
| ...normalizeSymbol(node, db, hc), | ||
| lineCount, | ||
| summary, | ||
| signature, | ||
| complexity: complexityMetrics, | ||
| callees, | ||
| callers, | ||
| relatedTests, | ||
| }; | ||
| }); | ||
| } | ||
|
|
||
| // ─── Exported functions ────────────────────────────────────────────────── | ||
|
|
||
| export function contextData(name, customDbPath, opts = {}) { |
There was a problem hiding this comment.
Duplicated resolveMethodViaHierarchy across modules
resolveMethodViaHierarchy is defined identically in both src/analysis/context.js (lines 292-316) and src/analysis/dependencies.js (lines 754-778). In the original queries.js, it was a single private helper shared by both contextData and fnDepsData — but the refactoring split each function into its own module, inadvertently duplicating the helper.
This is a code-smell introduced by the refactoring: two identical 25-line functions will diverge in maintenance. Consider extracting it to src/shared/ (e.g. src/shared/hierarchy.js) and importing it in both modules:
// src/shared/hierarchy.js
import { getClassHierarchy } from '../db.js';
export function resolveMethodViaHierarchy(db, methodName) {
// ...
}Then both context.js and dependencies.js can import from there.
There was a problem hiding this comment.
Fixed in d210a32. Extracted resolveMethodViaHierarchy to src/shared/hierarchy.js — both context.js and dependencies.js now import from the shared module instead of each having their own copy.
| import fs from 'node:fs'; | ||
| import path from 'node:path'; | ||
| import { | ||
| findCrossFileCallTargets, |
There was a problem hiding this comment.
Helper utilities imported from a peer analysis module
exports.js imports extractSignature, extractSummary, and safePath directly from ./context.js. This creates an unexpected cross-module dependency between two analysis modules. safePath in particular is a security-sensitive path-traversal guard, while extractSignature/extractSummary are generic file-reading utilities — none of them belong in a module named context.js.
The developer has already annotated them with // ─── Helpers (exported for use by exports.js), acknowledging the awkwardness. A cleaner long-term home would be src/shared/file-utils.js (or similar), so both analysis modules can import without creating an analysis → analysis dependency chain.
This is not a bug for this PR, but worth tracking as follow-up technical debt given the src/shared/ directory already exists for this purpose.
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 in d210a32. Moved safePath, readSourceRange, extractSummary, extractSignature, and isFileLikeTarget to src/shared/file-utils.js. Both context.js and exports.js now import from the shared module — no more analysis-to-analysis cross-dependency.
Resolve merge conflicts with main (v3.1.3 version bumps, changelog). Extract duplicated helpers into shared modules per Greptile review: - resolveMethodViaHierarchy → src/shared/hierarchy.js (was duplicated in context.js and dependencies.js) - safePath, readSourceRange, extractSummary, extractSignature, isFileLikeTarget → src/shared/file-utils.js (were in context.js, imported cross-module by exports.js) Impact: 61 functions changed, 138 affected
src/analysis/symbol-lookup.js
Outdated
| function _getFileHash(db, file) { | ||
| const row = db.prepare('SELECT hash FROM file_hashes WHERE file = ?').get(file); | ||
| return row ? row.hash : null; | ||
| } |
There was a problem hiding this comment.
Duplicate _getFileHash already exists as the private getFileHash in normalize.js
This private helper is identical — line for line — to the private getFileHash function inside src/shared/normalize.js:
// normalize.js (private, currently unexported)
function getFileHash(db, file) {
const row = db.prepare('SELECT hash FROM file_hashes WHERE file = ?').get(file);
return row ? row.hash : null;
}The refactoring already caught and fixed the resolveMethodViaHierarchy duplication (extracted to src/shared/hierarchy.js), but this identical two-line utility was missed. Exporting getFileHash from normalize.js and importing it here would eliminate the duplication and keep the single source of truth pattern consistent with the rest of the refactor:
// src/shared/normalize.js — add export keyword
export function getFileHash(db, file) { … }// src/analysis/symbol-lookup.js — replace the local copy
import { getFileHash } from '../shared/normalize.js';There was a problem hiding this comment.
Fixed in cd78487. Exported getFileHash from src/shared/normalize.js and replaced the duplicate _getFileHash in symbol-lookup.js with an import.
Address Greptile review: _getFileHash in symbol-lookup.js was identical to the private getFileHash in normalize.js. Export the original and import it instead of maintaining a duplicate. Impact: 2 functions changed, 6 affected
| const signature = fileLines ? extractSignature(fileLines, node.line) : null; | ||
|
|
||
| // Callees | ||
| const calleeRows = findCallees(db, node.id); | ||
| const filteredCallees = noTests ? calleeRows.filter((c) => !isTestFile(c.file)) : calleeRows; | ||
|
|
||
| const callees = filteredCallees.map((c) => { | ||
| const cLines = getFileLines(c.file); | ||
| const summary = cLines ? extractSummary(cLines, c.line) : null; | ||
| let calleeSource = null; | ||
| if (depth >= 1) { | ||
| calleeSource = readSourceRange(repoRoot, c.file, c.line, c.end_line); | ||
| } | ||
| return { | ||
| name: c.name, | ||
| kind: c.kind, | ||
| file: c.file, | ||
| line: c.line, |
There was a problem hiding this comment.
getFileLines closure duplicated across context.js and exports.js
The same fileCache + getFileLines pattern (lines 213–230 here) is copy-pasted verbatim into exports.js (lines 848–864). Now that both modules are separate and already import from src/shared/file-utils.js, this is a missed extraction opportunity — a createFileLinesReader(repoRoot) factory in file-utils.js would eliminate the duplication:
// src/shared/file-utils.js
export function createFileLinesReader(repoRoot) {
const cache = new Map();
return function getFileLines(file) {
if (cache.has(file)) return cache.get(file);
try {
const absPath = safePath(repoRoot, file);
if (!absPath) { cache.set(file, null); return null; }
const lines = fs.readFileSync(absPath, 'utf-8').split('\n');
cache.set(file, lines);
return lines;
} catch (e) {
debug(`getFileLines failed for ${file}: ${e.message}`);
cache.set(file, null);
return null;
}
};
}The same duplication also appears in explainData (line ~413) within the same file. Not a regression since this was already duplicated in queries.js, but the refactor is the right moment to consolidate it.
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 in 9cdd5a1. Extracted a createFileLinesReader(repoRoot) factory into src/shared/file-utils.js — all three identical closures (two in context.js, one in exports.js) now use it. Also removed the now-unused fs, debug, and safePath imports from both modules.
…s.js Deduplicate three identical getFileLines closures (two in context.js, one in exports.js) into a single createFileLinesReader factory in src/shared/file-utils.js. Remove now-unused fs/debug/safePath imports. Impact: 4 functions changed, 3 affected
| const row = db.prepare('SELECT hash FROM file_hashes WHERE file = ?').get(file); | ||
| return row ? row.hash : null; |
There was a problem hiding this comment.
Unexplained ALL_SYMBOL_KINDS re-export in normalize.js
normalize.js imports ALL_SYMBOL_KINDS from ../kinds.js and immediately re-exports it, with the comment "Re-export for consumers that import ALL_SYMBOL_KINDS from this module". However, none of the new analysis or shared modules import ALL_SYMBOL_KINDS from here — generators.js and symbol-lookup.js both import it directly from ../kinds.js. And queries.js re-exports ALL_SYMBOL_KINDS independently from ./kinds.js, not via this path.
This means the re-export is currently dead code, and the comment implies an intention that has no implementation behind it. It also muddies the responsibility of normalize.js, which should be concerned with symbol normalization and kind icons rather than exposing kind constant collections.
Consider either removing this re-export (it adds no backward-compat value since ALL_SYMBOL_KINDS was never exported from normalize.js before) or documenting a concrete consumer that warrants it.
| const row = db.prepare('SELECT hash FROM file_hashes WHERE file = ?').get(file); | |
| return row ? row.hash : null; | |
| import { ALL_SYMBOL_KINDS } from '../kinds.js'; |
(i.e., import it for normalizeSymbol's internal use if needed, but don't re-export it from here)
There was a problem hiding this comment.
Fixed in 69bea62. Removed the dead ALL_SYMBOL_KINDS import and re-export from normalize.js — no consumer used this path, everyone imports directly from kinds.js.
No consumer imports ALL_SYMBOL_KINDS from this module — all callers import directly from kinds.js. The re-export was dead code that muddied normalize.js's responsibility.
1 participant
Summary
queries.jsinto 9 focused modules undersrc/analysis/andsrc/shared/, completing roadmap item 3.4queries.jsbecomes a ~36-line barrel re-export — all existing importers unchangedNew files:
src/shared/normalize.jsnormalizeSymbol,kindIconsrc/shared/generators.jsiterListFunctions,iterRoles,iterWheresrc/analysis/symbol-lookup.jsfindMatchingNodes,queryNameData,whereData,listFunctionsData,childrenDatasrc/analysis/impact.jsimpactAnalysisData,fnImpactData,diffImpactData,diffImpactMermaidsrc/analysis/dependencies.jsfileDepsData,fnDepsData,pathDatasrc/analysis/module-map.jsmoduleMapData,statsDatasrc/analysis/context.jscontextData,explainDatasrc/analysis/exports.jsexportsDatasrc/analysis/roles.jsrolesDataTest plan