fix(perf): resolve config once in withRepo to avoid double loadConfig#2069
Open
carlos-alm wants to merge 1 commit into
Conversation
withRepo() opened its Repository via openRepo(), which independently resolves config for engine/busy-timeout selection, while its two callers that also need analysis-tuning config -- fnImpactData() via resolveAnalysisOpts(), briefData() via a direct loadConfig() call -- triggered a second, independent loadConfig() for the same DB path. Beyond the redundant work, the second call resolved its rootDir from process.cwd() instead of the resolved --db path, so a --db pointing at a different repo than cwd would silently read the wrong project's .codegraphrc.json for analysis.fnImpactDepth/analysis.briefCallerDepth -- the same correctness gap already fixed for withReadonlyDb()'s callers. Widen withRepo() to resolve config once and thread it through to both openRepo() (via a new, additive opts.config -- every other openRepo() caller is unaffected) and the callback, mirroring withReadonlyDb's (db, config) => T signature. fnImpactData and briefData now reuse opts.config ?? dbConfig instead of loading config again. dependencies.ts and implementations.ts, the other two withRepo callers, don't read config, so their existing (repo) => T callbacks are unaffected.
Contributor
Greptile SummaryThis PR resolves repository config once in the
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (1): Last reviewed commit: "fix(perf): resolve config once in withRe..." | Re-trigger Greptile |
Contributor
Codegraph Impact Analysis5 functions changed → 43 callers affected across 33 files
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
withRepo()(backingfn-impact.ts,brief.ts,dependencies.ts,implementations.ts) opens itsRepositoryviaopenRepo(), which internally resolves config viaresolveDbSettings()for engine/busy-timeout selection. Its two callers that also needed analysis-tuning config —fnImpactDataviaresolveAnalysisOpts(), andbriefDatavia a directloadConfig()call — triggered a second, independentloadConfig()for the same DB path.process.cwd()instead of the resolved--dbpath, so--db /other/repo/.codegraph/graph.dbwould silently read the wrong project's.codegraphrc.jsonforanalysis.fnImpactDepth/analysis.briefCallerDepth— the same correctness gap already fixed forwithReadonlyDb()'s callers (exports.ts,context.ts).withRepo()to resolve config once and thread it through to bothopenRepo()(via a new, additiveopts.config— every otheropenRepo()call site is unaffected) and the callback, mirroringwithReadonlyDb's(db, config) => Tsignature.fnImpactDataandbriefDatanow reuseopts.config ?? dbConfiginstead of callingloadConfig()again.dependencies.tsandimplementations.ts— the other twowithRepocallers — don't read config, so they're unaffected; TypeScript allows their existing(repo) => Tcallbacks to ignore the new second parameter.No native/Rust changes: this is a TS-only config-resolution optimization internal to
db/connection.ts/domain/analysis/query-helpers.tsorchestration — the native crate has no equivalent concept to mirror.Fixes #1941
Test plan
tests/unit/withrepo-config-resolution.test.ts: spies onloadConfigto assertwithRepo,fnImpactData, andbriefDataeach call it exactly once (previously twice for the latter two); also assertsanalysis.fnImpactDepth/analysis.briefCallerDepthare read from the--dbpath's rootDir even whenprocess.cwd()points elsewhere. Verified these tests fail against the pre-fix code (2 loadConfig calls / config not threaded) and pass after the fix.npx tsc --noEmit— cleannpm run lint— cleancodegraph diff-impact --staged -T— confirms only the intended 5 functions changed (resolveDbSettings,openRepo,briefData,fnImpactData,withRepo), 43 callers across 33 files, all backward-compatiblenpm test— 225/259 files and 3688/4134 tests pass. The remaining 20 failing files (6 individual test failures) all share one root cause unrelated to this change:Cannot find module '@optave/codegraph-darwin-arm64'— this worktree has no built native addon, a pre-existing, already-documented environment gap for isolated worktrees, not something this diff touches.