refactor(structure): share chunk-size statement cache with builder helpers - #1951
Conversation
Sorry, something went wrong.
…lpers batchUpdateRoles in features/structure.ts duplicated the WeakMap<db, Map<chunkSize, stmt>> cache-getter shape already extracted as getOrCreateBatchStmt in domain/graph/builder/helpers.ts. Extract the generic helper as cachedChunkStmt in db/repository/cached-stmt.ts (alongside the existing single-statement cachedStmt) and have both builder/helpers.ts and features/structure.ts depend on it, avoiding an awkward features/ -> domain/graph/builder/ dependency direction. As a side effect, the role statement cache is now module-scoped and keyed per-db like the node/edge/export caches, so repeated classifyNodeRoles calls against the same long-lived db connection (watch mode, MCP server) reuse compiled statements instead of recompiling them on every call. Impact: 6 functions changed, 21 affected
Greptile SummaryThis PR extracts the WeakMap<db, Map<chunkSize, stmt>> cache pattern—previously duplicated in builder/helpers.ts and features/structure.ts—into a shared cachedChunkStmt helper in src/db/repository/cached-stmt.ts. It also fixes a real performance regression by promoting roleStmtCache in batchUpdateRoles from a function-scoped Map (freshly allocated on every call) to a module-scoped WeakMap, so watch-mode and MCP-server workloads reuse compiled statements across repeated classifyNodeRoles invocations.
Confidence Score: 5/5Safe to merge — focused extraction of a duplicated pattern with no new logic and a real statement-reuse bug fixed. The refactoring is mechanical: cachedChunkStmt is a verbatim lift of getOrCreateBatchStmt, all callers are updated consistently, and the only behavioral change (module-scoped roleStmtCache) is the intended fix backed by two new regression tests. No existing logic paths are altered. Both structure.ts and builder/helpers.ts import cachedChunkStmt directly from the implementation file rather than through the db barrel — a minor style inconsistency worth tidying up. Important Files Changed
Reviews (1): Last reviewed commit: "refactor(structure): share chunk-size st..." | Re-trigger Greptile |
Sorry, something went wrong.
| import { getBuildMeta, getNodeId, setBuildMeta, testFilterSQL } from '../db/index.js'; | ||
| import { cachedChunkStmt } from '../db/repository/cached-stmt.js'; |
There was a problem hiding this comment.
Both structure.ts and builder/helpers.ts already import other symbols from the db barrel (../db/index.js), which now re-exports cachedChunkStmt. Adding a second import line that reaches into the implementation file directly (../db/repository/cached-stmt.js) creates two entry-points for the same module and diverges from the pattern used for every other db/ symbol in this file.
| import { getBuildMeta, getNodeId, setBuildMeta, testFilterSQL } from '../db/index.js'; | |
| import { cachedChunkStmt } from '../db/repository/cached-stmt.js'; | |
| import { cachedChunkStmt, getBuildMeta, getNodeId, setBuildMeta, testFilterSQL } from '../db/index.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!
Sorry, something went wrong.
| import { purgeFilesData } from '../../../db/index.js'; | ||
| import { cachedChunkStmt } from '../../../db/repository/cached-stmt.js'; |
There was a problem hiding this comment.
cachedChunkStmt is now re-exported from the db barrel (db/index.js), which this file already imports for purgeFilesData. Keeping the direct sub-path import (db/repository/cached-stmt.js) as a separate line is inconsistent with how every other db/ helper is consumed here.
| import { purgeFilesData } from '../../../db/index.js'; | |
| import { cachedChunkStmt } from '../../../db/repository/cached-stmt.js'; | |
| import { cachedChunkStmt, purgeFilesData } from '../../../db/index.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!
Sorry, something went wrong.
Codegraph Impact Analysis6 functions changed → 26 callers affected across 5 files
|
Sorry, something went wrong.
Fixes #1767
batchUpdateRoles in src/features/structure.ts duplicated the WeakMap<db, Map<chunkSize, stmt>> cache-getter shape already extracted as getOrCreateBatchStmt in src/domain/graph/builder/helpers.ts during a prior Titan grind pass.
Since features/ is a different domain than domain/graph/builder/, this extracts the generic helper into src/db/repository/cached-stmt.ts as cachedChunkStmt — a natural sibling of the existing single-statement cachedStmt helper already used across db/, domain/analysis/, etc. Both builder/helpers.ts and features/structure.ts now depend on this lower-level db/ module instead of crossing feature/domain boundaries directly. A matching ChunkStmtCache<TRow> type was added next to StmtCache<TRow> in types.ts.
As a side effect, the role statement cache in batchUpdateRoles is now module-scoped and keyed per-db (previously a fresh Map was allocated on every call), matching the semantics of the node/edge/export batch caches — repeated classifyNodeRoles calls against the same long-lived db connection (watch mode, MCP server) now reuse compiled statements instead of recompiling them every time.
No Rust changes: the native engine's role classification (crates/codegraph-core/src/graph/classifiers/roles.rs) already uses rusqlite's built-in prepare_cached, so this duplicate-cache pattern only existed on the TypeScript side.
Validation