mirror of
https://github.com/colbymchenry/codegraph.git
synced 2026-10-23 06:50:54 +08:00
fix(context): the busiest file is found even when 20 test or generated files outrank it (#2785)
Root cause: getDominantFile and getTopRouteFile took the top 20 files in SQL and only then dropped test and generated files in code. Twenty test files busier than the app's busiest file left nothing, so getDominantFile returned null and the context builder's core-directory boost never applied. Nineteen left the dominant file without its runner-up, and nextEdgeCount 0 passes the "3x the next file" test for a file that is not dominant at all. getTopRouteFile returned null the same way, and measured its 30% share against the routes of the 20 files in the window, not the project's. Fix: - DOMINANT_FILE_SQL ranks every file. The window saved no work: ORDER BY needs every group, so the aggregate runs to the end either way, and only one row per file comes back. - computeDominantFile filters the ranking 20 rows at a time until it has the dominant file and its runner-up. The generated-file lookup stays a bounded probe per batch. - getTopRouteFile counts every route file, with ties broken by path. Validation: - New dominant-file-window.test.ts, 6 tests, all failing on main: the issue's layout with 20 and with 19 test files, 25 header-declared generated files (past the first batch), and three route shapes. - The dominant-file cache, plan and allocation suites and the route-file detector test pass. - Seven of the eight real repos give the same dominant file before and after (express, flask, excalidraw, django, airflow, grpc-go, etcd). The closest to the window was airflow, with the runner-up at rank 16. - envoyproxy/go-control-plane crosses the window: 254 generated and test files outrank its first hand-written file. Main returns null; the fix returns xdsmatcher/pkg/matcher/matcher.go (167 edges, runner-up 165). - getTopRouteFile on main reported django's admin/sites.py at 8 of 16 routes and airflow's ui router at 65 of 65. Counting every route file gives 8 of 32 and 65 of 332, both under 30%, so null as documented. - Cost, median of 3 interleaved rounds of 5: getDominantFile on airflow (8,564 ranked files) 581 -> 587 ms, django 189 -> 181 ms, go-control-plane 277 -> 284 ms. getTopRouteFile on airflow 1.3 -> 2.5 ms. - Explore A/B, main vs fix: 996 queries on express, flask, excalidraw, django, airflow and go-control-plane, all byte-identical. Fixes #2578 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
800e582d55
commit
c46523079f
@@ -21,6 +21,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
|
||||
- In Python, a call through a module-level instance, like `_svc.delete(obj)` after `_svc = SceneService()`, now links to the method its class has or inherits, even when other classes have a method of that name. (#2582)
|
||||
- In Python, a call on a module imported from outside your project, like `subprocess.run(…)` after `import subprocess`, no longer links to a project method that happens to share its name. Thanks @Egrn for the report. (#2599)
|
||||
- `codegraph_explore`, the CLI `codegraph explore` and the prompt hook no longer stall for a minute or more on a large project after a full index; the step that finds the project's busiest file no longer slows to a crawl when a few files are huge, like a generated Android `R.java` (#2571). Thanks @yangfengfan2011.
|
||||
- `codegraph_explore` and `codegraph context` now find your project's busiest file, and favor the code around it, even when twenty or more test or generated files are busier still, such as a package of protobuf stubs. Before, the search looked only at the 20 busiest files and then set the tests and generated ones aside, so it could find no file at all, or judge one far ahead of the next when it was not. `getTopRouteFile()` in the library counts every file's routes the same way. Thanks @inth3shadows for the report. (#2578)
|
||||
- In JavaScript and TypeScript, a call written as the body of an `if`, `while`, `for` or `do` without braces, like `if (e.key === "Escape") setOpen(false);`, no longer links to a same-named property or method in another file when the name is the file's own, such as a `setOpen` from `useState` (#2585).
|
||||
- `codegraph sync` no longer fails with "FOREIGN KEY constraint failed" in a project whose code lazy-loads a workspace package, like Supabase Studio's `dynamic(() => import('icons').then((mod) => mod.EdgeFunctions))` or vben's `() => import('@vben/layouts').then((m) => m.IFrameView)`. Before, every sync after an edit in such a project rolled back, so the index stopped following your changes until a full re-index. Thanks @bompus for the report and @inth3shadows. (#2565)
|
||||
- In VB.NET, every member of a `Structure` is now indexed, including its fields, properties, methods, constructors and nested enums. Before, only the first member was, so the rest could not be found and their callers looked empty.
|
||||
|
||||
@@ -0,0 +1,154 @@
|
||||
/**
|
||||
* The dominant file and the top route file are chosen from every candidate,
|
||||
* not from a 20-row window that test and generated files can fill (#2578).
|
||||
*
|
||||
* Both queries took the top 20 files in SQL and only then dropped test and
|
||||
* generated files in code. Twenty test files with more in-file edges than the
|
||||
* app's busiest file left nothing, so `getDominantFile` returned null; nineteen
|
||||
* left the dominant file without its runner-up, and `nextEdgeCount: 0` passes
|
||||
* the context builder's "3x the next file" test for a file that is not
|
||||
* dominant at all. `getTopRouteFile` returned null the same way, and measured
|
||||
* its 30% share against the routes of the 20 files in the window instead of
|
||||
* the project's.
|
||||
*/
|
||||
import { describe, it, expect, afterEach } from 'vitest';
|
||||
import * as fs from 'fs';
|
||||
import * as os from 'os';
|
||||
import * as path from 'path';
|
||||
import CodeGraph from '../src/index';
|
||||
|
||||
let root: string | undefined;
|
||||
let cg: CodeGraph | undefined;
|
||||
|
||||
afterEach(() => {
|
||||
cg?.close();
|
||||
cg = undefined;
|
||||
if (root) fs.rmSync(root, { recursive: true, force: true });
|
||||
root = undefined;
|
||||
});
|
||||
|
||||
/** `n + 1` functions, each calling the next: n call edges inside the file. */
|
||||
function chain(prefix: string, n: number, header = ''): string {
|
||||
return header
|
||||
+ Array.from({ length: n }, (_, i) => `export function ${prefix}${i}() { return ${prefix}${i + 1}(); }`).join('\n')
|
||||
+ `\nexport function ${prefix}${n}() { return 0; }\n`;
|
||||
}
|
||||
|
||||
async function project(files: Record<string, string>): Promise<CodeGraph> {
|
||||
root = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-2578-'));
|
||||
for (const [rel, text] of Object.entries(files)) {
|
||||
fs.mkdirSync(path.dirname(path.join(root, rel)), { recursive: true });
|
||||
fs.writeFileSync(path.join(root, rel), text);
|
||||
}
|
||||
cg = await CodeGraph.init(root, { index: true });
|
||||
return cg;
|
||||
}
|
||||
|
||||
const queries = (g: CodeGraph) => (g as any).queries;
|
||||
|
||||
/** Edges between two symbols of `file`: what the dominant-file ranking counts. */
|
||||
function inFileEdges(g: CodeGraph, file: string): number {
|
||||
return ((g as any).db.db.prepare(`
|
||||
SELECT COUNT(*) AS n FROM edges e
|
||||
JOIN nodes s ON e.source = s.id JOIN nodes t ON e.target = t.id
|
||||
WHERE s.file_path = ? AND t.file_path = ?
|
||||
`).get(file, file) as { n: number }).n;
|
||||
}
|
||||
|
||||
/** The issue's layout: the app's two busiest files, below `tests` test files that each outrank both. */
|
||||
function appWithTests(tests: number): Record<string, string> {
|
||||
const files: Record<string, string> = {
|
||||
'src/core.ts': chain('core', 60),
|
||||
'src/second.ts': chain('second', 50),
|
||||
};
|
||||
for (let k = 0; k < tests; k++) files[`tests/t${k}.test.ts`] = chain(`t${k}_`, 70);
|
||||
return files;
|
||||
}
|
||||
|
||||
describe('getDominantFile looks past the test and generated files that outrank the app (#2578)', () => {
|
||||
it('finds the dominant file when 20 test files outrank it', async () => {
|
||||
const g = await project(appWithTests(20));
|
||||
const core = inFileEdges(g, 'src/core.ts');
|
||||
const second = inFileEdges(g, 'src/second.ts');
|
||||
// Fixture shape: every test file outranks the app's busiest file.
|
||||
expect(inFileEdges(g, 'tests/t0.test.ts')).toBeGreaterThan(core);
|
||||
expect(queries(g).getDominantFile()).toEqual({ filePath: 'src/core.ts', edgeCount: core, nextEdgeCount: second });
|
||||
}, 60_000);
|
||||
|
||||
it('keeps the runner-up when 19 test files leave room for only one app file in the top 20', async () => {
|
||||
const g = await project(appWithTests(19));
|
||||
const dominant = queries(g).getDominantFile();
|
||||
expect(dominant?.filePath).toBe('src/core.ts');
|
||||
// 0 here would pass the context builder's `edgeCount >= 3 * nextEdgeCount`
|
||||
// directory boost for a file with a close runner-up.
|
||||
expect(dominant?.nextEdgeCount).toBe(inFileEdges(g, 'src/second.ts'));
|
||||
expect(dominant!.edgeCount).toBeLessThan(3 * dominant!.nextEdgeCount);
|
||||
}, 60_000);
|
||||
|
||||
it('looks past files whose header says they are generated, beyond the first batch', async () => {
|
||||
const files: Record<string, string> = {
|
||||
'src/core.ts': chain('core', 60),
|
||||
'src/second.ts': chain('second', 50),
|
||||
};
|
||||
// Named like hand-written code; only the banner says otherwise, so the
|
||||
// index's generated flag is what has to drop them.
|
||||
for (let k = 0; k < 25; k++) files[`src/models/model${k}.ts`] = chain(`m${k}_`, 70, '// @generated by the model compiler. DO NOT EDIT.\n');
|
||||
const g = await project(files);
|
||||
expect(queries(g).getGeneratedPathsAmong(['src/models/model0.ts', 'src/core.ts'])).toEqual(new Set(['src/models/model0.ts']));
|
||||
expect(queries(g).getDominantFile()).toEqual({
|
||||
filePath: 'src/core.ts',
|
||||
edgeCount: inFileEdges(g, 'src/core.ts'),
|
||||
nextEdgeCount: inFileEdges(g, 'src/second.ts'),
|
||||
});
|
||||
}, 60_000);
|
||||
});
|
||||
|
||||
/** An Express router file with one handler per route. */
|
||||
function routes(paths: string[]): string {
|
||||
return `const express = require('express');
|
||||
const router = express.Router();
|
||||
|
||||
${paths.map((p, i) => `function handler${i}(req, res) { res.json({ at: '${p}' }); }`).join('\n')}
|
||||
|
||||
${paths.map((p, i) => `router.get('${p}', handler${i});`).join('\n')}
|
||||
|
||||
module.exports = router;
|
||||
`;
|
||||
}
|
||||
|
||||
const EXPRESS_PACKAGE = JSON.stringify({ name: 'shop', dependencies: { express: '^4.19.0' } });
|
||||
|
||||
describe('getTopRouteFile counts every route file (#2578)', () => {
|
||||
it('finds the app\'s routes file when 20 test servers hold more routes', async () => {
|
||||
const files: Record<string, string> = {
|
||||
'package.json': EXPRESS_PACKAGE,
|
||||
'src/routes.js': routes(['/orders', '/orders/:id', '/orders/:id/items', '/orders/:id/status']),
|
||||
};
|
||||
for (let k = 0; k < 20; k++) files[`tests/server${k}.test.js`] = routes(['/a', '/b', '/c', '/d', '/e'].map((p) => `/t${k}${p}`));
|
||||
const g = await project(files);
|
||||
expect(g.getTopRouteFile()).toEqual({ filePath: 'src/routes.js', routeCount: 4, totalRoutes: 4 });
|
||||
}, 60_000);
|
||||
|
||||
it('measures the share against every route, not the 20 busiest files', async () => {
|
||||
const files: Record<string, string> = {
|
||||
'package.json': EXPRESS_PACKAGE,
|
||||
'src/routes.js': routes(Array.from({ length: 30 }, (_, i) => `/r${i}`)),
|
||||
};
|
||||
for (let k = 0; k < 40; k++) files[`src/features/f${k}.js`] = routes([`/f${k}`]);
|
||||
const g = await project(files);
|
||||
// 30 of 70 routes: a 43% share, and the total is all 70.
|
||||
expect(g.getTopRouteFile()).toEqual({ filePath: 'src/routes.js', routeCount: 30, totalRoutes: 70 });
|
||||
}, 60_000);
|
||||
|
||||
it('answers null when routing is diffuse across more than 20 files', async () => {
|
||||
const files: Record<string, string> = {
|
||||
'package.json': EXPRESS_PACKAGE,
|
||||
'src/routes.js': routes(Array.from({ length: 10 }, (_, i) => `/r${i}`)),
|
||||
};
|
||||
for (let k = 0; k < 40; k++) files[`src/features/f${k}.js`] = routes([`/f${k}`]);
|
||||
const g = await project(files);
|
||||
// 10 of 50 routes is 20%, under the 30% bar. Counting only the 20 busiest
|
||||
// files made it 10 of 29.
|
||||
expect(g.getTopRouteFile()).toBeNull();
|
||||
}, 60_000);
|
||||
});
|
||||
+47
-16
@@ -257,6 +257,12 @@ function rowToFileRecord(row: FileRow): FileRecord {
|
||||
* under-count nodes per file and pick that plan, and a few huge files (a
|
||||
* generated R.java) then cost ~100 s (#2571). Ties go to the path, so the
|
||||
* plan never decides between equal counts.
|
||||
*
|
||||
* Every file, not a top-N window: the test and generated files the caller
|
||||
* drops can fill any window, and then the real dominant file, or its
|
||||
* runner-up, was never among the rows (#2578). The window saved nothing
|
||||
* either: ORDER BY needs every group, so the aggregate runs to the end
|
||||
* whatever the LIMIT; only the rows handed back differ, one per file.
|
||||
*/
|
||||
export const DOMINANT_FILE_SQL = `
|
||||
SELECT n.file_path AS file_path, COUNT(*) AS edge_count
|
||||
@@ -266,9 +272,15 @@ export const DOMINANT_FILE_SQL = `
|
||||
WHERE n.file_path = m.file_path
|
||||
GROUP BY n.file_path
|
||||
ORDER BY edge_count DESC, n.file_path
|
||||
LIMIT 20
|
||||
`;
|
||||
|
||||
/**
|
||||
* Ranked candidates the low-value filter is asked about at a time, in
|
||||
* `computeDominantFile`: the generated flag is a database lookup, so it is
|
||||
* made per batch rather than for every file with an in-file edge.
|
||||
*/
|
||||
const DOMINANT_FILE_BATCH = 20;
|
||||
|
||||
/**
|
||||
* Query builder for the knowledge graph database
|
||||
*/
|
||||
@@ -1189,8 +1201,8 @@ export class QueryBuilder {
|
||||
|
||||
private computeDominantFile(): { filePath: string; edgeCount: number; nextEdgeCount: number } | null {
|
||||
if (!this.stmts.getDominantFile) {
|
||||
// Pull top 20 candidates; we then filter out test/generated files
|
||||
// in code (regex-grade matching that SQL LIKE can't express). The
|
||||
// Every file ranked; test/generated files are then filtered out in
|
||||
// code (regex-grade matching that SQL LIKE can't express). The
|
||||
// generated-file filter is critical — without it, etcd's
|
||||
// `api/etcdserverpb/rpc.pb.go` (1916 in-file edges, generated
|
||||
// protobuf stub) outranks the real `server/etcdserver/server.go`
|
||||
@@ -1200,13 +1212,26 @@ export class QueryBuilder {
|
||||
this.stmts.getDominantFile = this.db.prepare(DOMINANT_FILE_SQL);
|
||||
}
|
||||
const rows = this.stmts.getDominantFile.all() as Array<{ file_path: string; edge_count: number }>;
|
||||
const generated = this.getGeneratedPathsAmong(rows.map(r => r.file_path));
|
||||
const filtered = rows.filter(r => !isLowValueFile(r.file_path, generated));
|
||||
if (filtered.length === 0 || filtered[0]!.edge_count < 20) return null;
|
||||
// The first two files the filter keeps, in rank order: the dominant file
|
||||
// and its runner-up. Filtering a 20-row window instead returned null once
|
||||
// 20 test files outranked the real code, and an understated runner-up
|
||||
// (0) once 19 did, which passes the caller's "3x the next file" test for
|
||||
// a file that is not dominant at all (#2578).
|
||||
const kept: Array<{ file_path: string; edge_count: number }> = [];
|
||||
for (let i = 0; i < rows.length && kept.length < 2; i += DOMINANT_FILE_BATCH) {
|
||||
const batch = rows.slice(i, i + DOMINANT_FILE_BATCH);
|
||||
const generated = this.getGeneratedPathsAmong(batch.map(r => r.file_path));
|
||||
for (const r of batch) {
|
||||
if (isLowValueFile(r.file_path, generated)) continue;
|
||||
kept.push(r);
|
||||
if (kept.length === 2) break;
|
||||
}
|
||||
}
|
||||
if (kept.length === 0 || kept[0]!.edge_count < 20) return null;
|
||||
return {
|
||||
filePath: filtered[0]!.file_path,
|
||||
edgeCount: filtered[0]!.edge_count,
|
||||
nextEdgeCount: filtered[1]?.edge_count ?? 0,
|
||||
filePath: kept[0]!.file_path,
|
||||
edgeCount: kept[0]!.edge_count,
|
||||
nextEdgeCount: kept[1]?.edge_count ?? 0,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -1221,6 +1246,12 @@ export class QueryBuilder {
|
||||
* Excludes test/generated files from candidacy. Returns null if there
|
||||
* are fewer than 3 non-test routes total, or if no file holds at least
|
||||
* 30% of them (diffuse routing → no single answer file).
|
||||
*
|
||||
* Every route file is counted. A 20-row window, filtered afterwards,
|
||||
* returned null once 20 test files held more routes than the app's own,
|
||||
* and measured the 30% share against the routes of those 20 files rather
|
||||
* than of the project (#2578). One row per file that has a route, so the
|
||||
* window saved next to nothing.
|
||||
*/
|
||||
getTopRouteFile(): { filePath: string; routeCount: number; totalRoutes: number } | null {
|
||||
if (!this.stmts.getTopRouteFile) {
|
||||
@@ -1229,8 +1260,7 @@ export class QueryBuilder {
|
||||
FROM nodes
|
||||
WHERE kind = 'route'
|
||||
GROUP BY file_path
|
||||
ORDER BY cnt DESC
|
||||
LIMIT 20
|
||||
ORDER BY cnt DESC, file_path
|
||||
`);
|
||||
}
|
||||
const rows = this.stmts.getTopRouteFile.all() as Array<{ file_path: string; cnt: number }>;
|
||||
@@ -3455,11 +3485,12 @@ export class QueryBuilder {
|
||||
/**
|
||||
* Which of `filePaths` the index flagged as tool-generated (schema v9+).
|
||||
*
|
||||
* Bounded-lookup by design: every consumer already holds a short candidate
|
||||
* list (a ranked file group, an FTS result page, a LIMIT-20 aggregate), so
|
||||
* this stays a partial-index probe over a handful of paths — no whole-repo
|
||||
* set to materialize, and no cache to invalidate, which means a ranking call
|
||||
* can never serve a verdict the last sync already replaced.
|
||||
* Bounded-lookup by design: every consumer already holds a candidate list
|
||||
* (a ranked file group, an FTS result page, a batch of the dominant-file
|
||||
* ranking, the files that hold a route), so this stays a partial-index
|
||||
* probe over those paths — no whole-repo set to materialize, and no cache
|
||||
* to invalidate, which means a ranking call can never serve a verdict the
|
||||
* last sync already replaced.
|
||||
*
|
||||
* Returns ONLY the content/index signal; callers union it with
|
||||
* {@link isGeneratedFile} so pre-v9 databases (column present, all zeros
|
||||
|
||||
Reference in New Issue
Block a user