Skip to content

Address review findings #98

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lzehrung merged 7 commits into from
May 20, 2026
Merged

Address review findings #98

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2bc2242
Address impact and graph review findings
lzehrung May 20, 2026
04b87f9
Consolidate import extraction paths
lzehrung May 20, 2026
3671bd8
Document second-pass complexity review
lzehrung May 20, 2026
fa42adb
Document third-pass complexity review
lzehrung May 20, 2026
7250151
Document fourth-pass complexity review
lzehrung May 20, 2026
90f9f0f
Address PR review comments
lzehrung May 20, 2026
f84cc62
Include type-only state in streaming dedupe
lzehrung May 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
191 changes: 191 additions & 0 deletions REVIEW_PLAN.md
View file
Open in desktop

Large diffs are not rendered by default.

15 changes: 5 additions & 10 deletions src/graphs/specifiers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
import { type LanguageSupport } from "../languages.js";
import {
parseCsharpUsingDirective,
parseKotlinImportStatement,
parsePhpImportStatement,
parseRustImportStatement,
} from "../languages/importStatementParsers.js";
Expand Down Expand Up @@ -60,12 +61,6 @@ function isHtmlLikeLanguage(languageId: string, filePath?: string): boolean {
return !!filePath && filePath.toLowerCase().endsWith(".astro");
}

function extractKotlinImportSpecifier(statementText: string): string | null {
const match = statementText.match(/^\s*import\s+([A-Za-z_][\w.]*(?:\.\*)?)(?:\s+as\s+[A-Za-z_][\w]*)?\s*$/m);
if (!match?.[1]) return null;
return match[1].endsWith(".*") ? match[1].slice(0, -2) : match[1];
}

function extractPhpQualifiedSpecifiersFromTree(source: string, tree: SyntaxTreeLike): ModuleSpecifier[] {
const specifiers: ModuleSpecifier[] = [];
const seen = new Set<string>();
Expand Down Expand Up @@ -318,8 +313,8 @@ export function collectModuleSpecifiersFromSource(
(support.id === "ts" || support.id === "tsx") &&
(/\b(import|export)\s+type\b/.test(stmtText) || /^\s*declare\s+module\s+["']/.test(stmtText));
if (support.id === "kotlin") {
const spec = extractKotlinImportSpecifier(stmtText);
if (spec) out.push({ spec, typeOnly: false });
const parsed = parseKotlinImportStatement(stmtText);
if (parsed) out.push({ spec: parsed.from, typeOnly: false });
continue;
}
if (support.id === "rust") {
Expand Down Expand Up @@ -405,8 +400,8 @@ export function collectModuleSpecifiersFromSource(
(support.id === "ts" || support.id === "tsx") &&
(/\b(import|export)\s+type\b/.test(stmtText) || /^\s*declare\s+module\s+["']/.test(stmtText));
if (support.id === "kotlin") {
const spec = extractKotlinImportSpecifier(stmtText);
if (spec) out.push({ spec, typeOnly: false });
const parsed = parseKotlinImportStatement(stmtText);
if (parsed) out.push({ spec: parsed.from, typeOnly: false });
continue;
}
if (support.id === "rust") {
Expand Down
126 changes: 44 additions & 82 deletions src/graphs/symbol-graph-detailed.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,48 @@ export async function buildSymbolGraphDetailed(
"optional_chain",
sup.id === "python" ? "attribute" : "",
]);
const resolveMemberChainTarget = (chainNode: SyntaxNodeLike): SymbolDef | null => {
const names: string[] = [];
let current: SyntaxNodeLike | null = chainNode;
let base: SyntaxNodeLike | null = null;
const pushProp = (propNode: SyntaxNodeLike | null) => {
if (!propNode) return;
if (propertyIdentifierTypes.includes(propNode.type)) names.push(sliceText(propNode, src));
else if (propNode.type === "string") names.push(unquote(sliceText(propNode, src)));
else if (propNode.type === "identifier") {
const keyName = sliceText(propNode, src);
const value = constStringOf.get(keyName);
if (typeof value === "string") names.push(value);
}
};
while (current && optionalMemberTypes.has(current.type)) {
if (current.type === "subscript_expression") {
base = current.child(0) ?? base;
const indexNode = current.child(2);
pushProp(indexNode);
current = base;
} else if (
current.type === memberExpressionType ||
current.type === "optional_member_expression" ||
current.type === "attribute"
) {
base = current.child(0) ?? base;
const propNode =
current.childForFieldName?.("property") ?? current.child(2) ?? current.childForFieldName?.("attribute");
pushProp(propNode);
current = base;
} else if (current.type === "optional_chain") {
current = current.child(0);
} else {
break;
}
}
if (!current || !isIdentifierType(sup, current.type)) return null;
const alias = sliceText(current, src);
const targetFile = aliasToTargetModule.get(alias);
if (!targetFile || !names.length) return null;
return resolveMemberPathFromModule(targetFile, names);
};
const walkCollect = (node: SyntaxNodeLike) => {
if (
node.type === "function_declaration" ||
Expand Down Expand Up @@ -403,46 +445,7 @@ export async function buildSymbolGraphDetailed(
node.child(0);

const tryResolveChain = (node: SyntaxNodeLike, fromId?: string, label = "uses") => {
const names: string[] = [];
let current: SyntaxNodeLike | null = node;
let base: SyntaxNodeLike | null = null;
const pushProp = (propNode: SyntaxNodeLike | null) => {
if (!propNode) return;
if (propertyIdentifierTypes.includes(propNode.type)) names.push(sliceText(propNode, src));
else if (propNode.type === "string") names.push(unquote(sliceText(propNode, src)));
else if (propNode.type === "identifier") {
const keyName = sliceText(propNode, src);
const value = constStringOf.get(keyName);
if (typeof value === "string") names.push(value);
}
};
while (current && optionalMemberTypes.has(current.type)) {
if (current.type === "subscript_expression") {
base = current.child(0) ?? base;
const indexNode = current.child(2);
pushProp(indexNode);
current = base;
} else if (
current.type === memberExpressionType ||
current.type === "optional_member_expression" ||
current.type === "attribute"
) {
base = current.child(0) ?? base;
const propNode =
current.childForFieldName?.("property") ?? current.child(2) ?? current.childForFieldName?.("attribute");
pushProp(propNode);
current = base;
} else if (current.type === "optional_chain") {
current = current.child(0);
} else {
break;
}
}
if (!current || !isIdentifierType(sup, current.type)) return false;
const alias = sliceText(current, src);
const targetFile = aliasToTargetModule.get(alias);
if (!targetFile || !names.length) return false;
const targetDef = resolveMemberPathFromModule(targetFile, names);
const targetDef = resolveMemberChainTarget(node);
if (targetDef && fromId) {
const toId = defNodeId(targetDef);
if (!nodes.has(toId)) nodes.set(toId, nodeForDef(targetDef));
Expand Down Expand Up @@ -544,48 +547,7 @@ export async function buildSymbolGraphDetailed(

const walkForMembers = (node: SyntaxNodeLike) => {
const tryResolveChainLocal = (chainNode: SyntaxNodeLike) => {
const names: string[] = [];
let current: SyntaxNodeLike | null = chainNode;
let base: SyntaxNodeLike | null = null;
const pushProp = (propNode: SyntaxNodeLike | null) => {
if (!propNode) return;
if (propertyIdentifierTypes.includes(propNode.type)) names.push(sliceText(propNode, src));
else if (propNode.type === "string") names.push(unquote(sliceText(propNode, src)));
else if (propNode.type === "identifier") {
const keyName = sliceText(propNode, src);
const value = constStringOf.get(keyName);
if (typeof value === "string") names.push(value);
}
};
while (current && optionalMemberTypes.has(current.type)) {
if (current.type === "subscript_expression") {
base = current.child(0) ?? base;
const indexNode = current.child(2);
pushProp(indexNode);
current = base;
} else if (
current.type === memberExpressionType ||
current.type === "optional_member_expression" ||
current.type === "attribute"
) {
base = current.child(0) ?? base;
const propNode =
current.childForFieldName?.("property") ??
current.child(2) ??
current.childForFieldName?.("attribute");
pushProp(propNode);
current = base;
} else if (current.type === "optional_chain") {
current = current.child(0);
} else {
break;
}
}
if (!current || !isIdentifierType(sup, current.type)) return;
const alias = sliceText(current, src);
const targetFile = aliasToTargetModule.get(alias);
if (!targetFile || !names.length) return;
const targetDef = resolveMemberPathFromModule(targetFile, names);
const targetDef = resolveMemberChainTarget(chainNode);
if (targetDef) {
const toId = defNodeId(targetDef);
if (!nodes.has(toId)) nodes.set(toId, nodeForDef(targetDef));
Expand Down
7 changes: 6 additions & 1 deletion src/impact/analyzer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ const severityWeightKeys: ReadonlyArray<keyof SeverityWeights> = [
"depthDecay",
];

function referenceScanLimitForKeptRefs(maxRefs: number): number {
return Math.max(maxRefs + 50, maxRefs * 4);
}

function normalizeSeverityWeights(weights: SeverityWeights): SeverityWeights {
const normalized: SeverityWeights = { ...DEFAULT_SEVERITY_WEIGHTS };
const invalidEntries: string[] = [];
Expand Down Expand Up @@ -205,8 +209,9 @@ export async function analyzeImpact(
...(refBlockMaxLines !== undefined && {
blockMaxLines: refBlockMaxLines,
}),
maxReferences: referenceScanLimitForKeptRefs(maxRefs),
}
: undefined,
: { maxReferences: referenceScanLimitForKeptRefs(maxRefs) },
);

if (refs.status === "ok") {
Expand Down
55 changes: 26 additions & 29 deletions src/impact/context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { FileId } from "../types.js";
import type { ProjectIndex } from "../indexer.js";
import { buildSymbolGraphDetailed } from "../graphs.js";
import type { SymbolEdge } from "../graphs.js";
import { buildGraphAdjacency, getForwardNeighbors, getReverseNeighbors } from "../graphs/adjacency.js";
import { createGraphFileResolver } from "./path.js";
import { compileTestPatterns, createIndexTestFileMatcher } from "./testPatterns.js";

Expand Down Expand Up @@ -67,6 +68,8 @@ function collectFileSubgraph(
const edges: Array<{ from: FileId; to: FileId; typeOnly?: boolean }> = [];
const visited = new Set<FileId>();
const queue: Array<{ file: FileId; depth: number }> = [];
const adjacency = index.graphAdjacency ?? buildGraphAdjacency(index.graph);
const typeOnlyByPair = new Map<string, { allTypeOnly: boolean; hasTypeOnlyMetadata: boolean }>();

// Initialize with impacted files
for (const file of impactedFiles) {
Expand All @@ -75,21 +78,13 @@ function collectFileSubgraph(
queue.push({ file, depth: 0 });
}

// Build forward and reverse dependency maps for efficient traversal
const forwardDeps = new Map<FileId, FileId[]>(); // file -> files it depends on
const reverseDeps = new Map<FileId, FileId[]>(); // file -> files that depend on it

for (const edge of index.graph.edges) {
if (edge.to.type === "file") {
// Forward: A -> B means A depends on B
const deps = forwardDeps.get(edge.from) || [];
deps.push(edge.to.path);
forwardDeps.set(edge.from, deps);

// Reverse: A -> B means B is depended on by A
const revDeps = reverseDeps.get(edge.to.path) || [];
revDeps.push(edge.from);
reverseDeps.set(edge.to.path, revDeps);
const key = `${edge.from}\0${edge.to.path}`;
const current = typeOnlyByPair.get(key) ?? { allTypeOnly: true, hasTypeOnlyMetadata: false };
current.allTypeOnly = current.allTypeOnly && edge.typeOnly === true;
current.hasTypeOnlyMetadata = current.hasTypeOnlyMetadata || edge.typeOnly !== undefined;
typeOnlyByPair.set(key, current);
}
}

Expand All @@ -100,31 +95,42 @@ function collectFileSubgraph(
if (depth >= hops) continue;

// Add forward dependencies (files this file depends on)
const deps = forwardDeps.get(file) || [];
const deps = getForwardNeighbors(adjacency, file);
for (const dep of deps) {
if (!visited.has(dep)) {
visited.add(dep);
nodes.add(dep);
queue.push({ file: dep, depth: depth + 1 });
}
edges.push({ from: file, to: dep });
edges.push(edgeFor(file, dep, typeOnlyByPair));
}

// Add reverse dependencies (files that depend on this file)
const revDeps = reverseDeps.get(file) || [];
const revDeps = getReverseNeighbors(adjacency, file);
for (const revDep of revDeps) {
if (!visited.has(revDep)) {
visited.add(revDep);
nodes.add(revDep);
queue.push({ file: revDep, depth: depth + 1 });
}
edges.push({ from: revDep, to: file });
edges.push(edgeFor(revDep, file, typeOnlyByPair));
}
}

return { nodes, edges };
}

function edgeFor(
from: FileId,
to: FileId,
typeOnlyByPair: ReadonlyMap<string, { allTypeOnly: boolean; hasTypeOnlyMetadata: boolean }>,
): { from: FileId; to: FileId; typeOnly?: boolean } {
const typeOnly = typeOnlyByPair.get(`${from}\0${to}`);
if (!typeOnly) return { from, to };
if (typeOnly.allTypeOnly) return { from, to, typeOnly: true };
return typeOnly.hasTypeOnlyMetadata ? { from, to, typeOnly: false } : { from, to };
}

async function collectSymbolNeighbors(
index: ProjectIndex,
changedSymbolIds: string[],
Expand Down Expand Up @@ -251,20 +257,11 @@ export function listCandidateTestFiles(
const candidates = new Map<FileId, CandidateTestFile>();
const resolveGraphFile = createGraphFileResolver(index.graph.nodes);
const resolvedChangedFiles = changedFiles.map((file) => resolveGraphFile(file));
const adjacency = index.graphAdjacency ?? buildGraphAdjacency(index.graph);
// Default test patterns (can be extended by caller)
const allPatterns = compileTestPatterns(testPatterns);
const isIndexTestFile = createIndexTestFileMatcher(index, allPatterns, projectRoot, resolvedChangedFiles);

// Build reverse dependency map: file -> files that depend on it
const reverseDeps = new Map<FileId, FileId[]>();
for (const edge of index.graph.edges) {
if (edge.to.type === "file") {
const deps = reverseDeps.get(edge.to.path) || [];
deps.push(edge.from);
reverseDeps.set(edge.to.path, deps);
}
}

// Find test files that import changed symbols directly
const symbolFiles = new Set<FileId>();
for (const symbolId of changedSymbolIds) {
Expand All @@ -274,7 +271,7 @@ export function listCandidateTestFiles(
}

for (const file of symbolFiles) {
const dependents = reverseDeps.get(file) || [];
const dependents = getReverseNeighbors(adjacency, file);
for (const dependent of dependents) {
if (isIndexTestFile(dependent)) {
candidates.set(dependent, {
Expand All @@ -288,7 +285,7 @@ export function listCandidateTestFiles(

// Find test files that depend on changed files (lower confidence)
for (const changedFile of resolvedChangedFiles) {
const dependents = reverseDeps.get(changedFile) || [];
const dependents = getReverseNeighbors(adjacency, changedFile);
for (const dependent of dependents) {
if (isIndexTestFile(dependent) && !candidates.has(dependent)) {
candidates.set(dependent, {
Expand Down
Loading
Loading