Skip to content

perf(query): fix diffImpact latency regression (#904) #905

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

Open
carlos-alm wants to merge 4 commits into
base: main
Choose a base branch
from
+28 −1
Open

perf(query): fix diffImpact latency regression (#904) #905

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dbf3a6f
fix(native): strip pre-release suffix in semverCompare
carlos-alm Apr 9, 2026
d99013f
perf(query): short-circuit diffImpact when no functions affected
carlos-alm Apr 9, 2026
14bf7c4
fix(query): preserve boundary checks in diffImpact short-circuit (#905)
carlos-alm Apr 9, 2026
f201774
Merge branch 'main' into fix/semver-prerelease
carlos-alm Apr 9, 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
29 changes: 28 additions & 1 deletion src/domain/analysis/diff-impact.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,33 @@ export function diffImpactData(
}

const affectedFunctions = findAffectedFunctions(db, changedRanges, noTests);

// Short-circuit: when no function-level changes detected, skip expensive
// lookups (BFS, co-change, ownership). Boundary checks are preserved
// because they are file-scoped and can surface real violations even when
// no function bodies were touched (e.g. import or type-alias changes).
if (affectedFunctions.length === 0) {
const { boundaryViolations, boundaryViolationCount } = checkBoundaryViolations(
db,
changedRanges,
noTests,
{ ...opts, config },
repoRoot,
);
const base = {
changedFiles: changedRanges.size,
newFiles: [...newFiles],
affectedFunctions: [] as unknown[],
affectedFiles: [] as string[],
historicallyCoupled: [] as unknown[],
ownership: null,
boundaryViolations,
boundaryViolationCount,
summary: null as null,
};
return paginateResult(base, 'affectedFunctions', { limit: opts.limit, offset: opts.offset });
}
Comment on lines +315 to +335
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Boundary checks silently skipped for file-only changes

checkBoundaryViolations is scoped to files (scopeFiles: [...changedRanges.keys()]), not functions — so it can surface violations from pure import/type-alias changes even when no function body was touched. The new early return hardcodes boundaryViolations: [] / boundaryViolationCount: 0 for that case, silently suppressing any real violations.

If a caller uses summary.boundaryViolationCount (rather than checking functionsChanged first), it will see 0 even when architectural boundaries are actually violated. The PR description asserts callers return early on empty affectedFunctions, but that claim should be validated before skipping a check that is semantically independent of function-level changes. At minimum, consider preserving the boundary check in this branch:

const { boundaryViolations, boundaryViolationCount } = checkBoundaryViolations(
  db,
  changedRanges,
  noTests,
  { ...opts, config },
  repoRoot,
);
const base = {
  ...
  boundaryViolations,
  boundaryViolationCount,
  summary: {
    ...
    boundaryViolationCount,
  },
};
return paginateResult(base, 'affectedFunctions', { limit: opts.limit, offset: opts.offset });

Copy link
Copy Markdown
Contributor Author

@carlos-alm carlos-alm Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 14bf7c4 — the short-circuit path now preserves the boundary check since checkBoundaryViolations is file-scoped (via scopeFiles: [...changedRanges.keys()]) and can surface real violations even when no function bodies were touched. The return shape is also aligned: summary: null now matches the two existing early-exit paths, so callers that branch on summary === null behave consistently.


const includeImplementors = opts.includeImplementors !== false;
const { functionResults, allAffected } = buildFunctionImpactResults(
db,
Expand All @@ -325,7 +352,7 @@ export function diffImpactData(
db,
changedRanges,
noTests,
opts,
{ ...opts, config },
repoRoot,
);

Expand Down
Loading