refactor: split impact.ts into fn-impact and diff-impact modules

[carlos-alm]

Summary

• Splits impact.ts (721 SLOC) into fn-impact.ts and diff-impact.ts
• Moves mermaid diagram generation to presentation/diff-impact-mermaid.ts
• Reduces largest domain/analysis file below SLOC threshold

Titan Audit Context

• Phase: decomposition (Forge Phase 2)
• Domain: domain/analysis
• Commits: 1
• Depends on: none

Changes

• src/domain/analysis/impact.ts -- reduced to re-exports and shared utilities
• src/domain/analysis/fn-impact.ts -- extracted function-level impact analysis (BFS callers)
• src/domain/analysis/diff-impact.ts -- extracted diff-based impact analysis (git diff parsing)
• src/presentation/diff-impact-mermaid.ts -- extracted mermaid diagram rendering

Metrics Impact

• impact.ts SLOC: 721 -> split into ~350 + ~350
• Blast radius: 15 direct importers need path updates (handled via re-exports)

Test plan

• CI passes (lint + build + tests)
• codegraph check --cycles --boundaries passes
• No new functions above complexity thresholds

[claude]

Claude finished @carlos-alm's task —— View job

---

Reviewing PR: Split impact.ts into fn-impact and diff-impact modules

Conducting comprehensive 5-phase review analysis:

Todo List:

• Phase 1: Root Cause Analysis - Understand the problem being solved
• Read all changed files to understand the refactoring
• Phase 2: Configuration Change Detection
• Phase 3: Test Integrity Analysis
• Phase 4: Code Quality Assessment
• Phase 5: Backlog Compliance Evaluation
• Provide final assessment and recommendation

Phase 1: Root Cause Analysis ✅

Problem Being Solved: Large monolithic impact.ts file (721 SLOC) violating maintainability threshold, making it hard to navigate and modify impact analysis functionality.

Why This Approach: Clean separation of concerns - function-level impact analysis (fn-impact.ts) vs git diff-based impact analysis (diff-impact.ts), with presentation logic properly moved to presentation/ layer.

Backward Compatibility: Maintained via re-export barrel pattern in the original impact.ts file - all existing imports continue to work.

Phase 2: Configuration Change Detection ✅

No Configuration Changes: This is a pure code refactoring - no biome rules, test configurations, or build settings modified.

Phase 3: Test Integrity ✅

Test Coverage Preserved: Found 6 test files referencing the moved functions. All imports go through the barrel file (impact.js) which maintains the exact same export surface, so no test updates are needed.

Phase 4: Code Quality Assessment ✅

Strengths:

• Clean separation of concerns: Function-level impact (fn-impact.ts) vs diff-based impact (diff-impact.ts)
• Proper layering: Mermaid presentation logic correctly moved to presentation/ directory
• Zero breaking changes: Perfect re-export barrel maintains API compatibility
• Security fix preserved: Line 257 comment documents shell injection fix using execFileSync
• Performance optimizations maintained: WeakMap caching for implements edges, statement caching

Code Quality Issues Found:

• Missing error handling: bfsTransitiveCallers line 143 uses non-null assertion (!) without null checks
• WeakMap usage: Cache keying on database objects could cause memory leaks if db instances accumulate
• Type safety: Several as type assertions that could fail at runtime (lines 72, 74, 100, 114, 167)

Architecture Compliance:

• ✅ Follows established domain/analysis pattern
• ✅ Presentation logic correctly separated
• ✅ Database operations properly isolated

Phase 5: Backlog Compliance Evaluation

Root Cause Analysis

Problem Being Solved: Reducing file complexity from 721 SLOC to maintainable modules (<350 SLOC each) to improve navigation and reduce cognitive load during development.
Why This Approach: Standard separation of concerns refactoring - function-level vs diff-based impact analysis with proper presentation layer separation.
Risk Assessment: Very low risk - backward compatibility maintained, no functionality changes, proper test coverage exists.

Backlog Compliance

• Zero-dep: ✓ — No new dependencies added, pure refactoring
• Foundation-aligned: ✓ — Maintains all principles, improves maintainability
• Problem-fit: 3 — Code organization improvement, indirectly helps AI navigation but doesn't directly address the core "lost AI" problem
• Breaking: No — Exact same export surface maintained via re-exports
• Tier: 1

Critical Concerns

None - This is a well-executed refactoring with proper backward compatibility.

Minor improvements suggested:

1. Add null checks before non-null assertions in bfsTransitiveCallers
2. Consider documentation for the WeakMap caching strategy
3. Add JSDoc comments for the main exported functions

Final Recommendation

• Rating: ⭐⭐⭐⭐☆ (4/5)
• Action: APPROVE
• Reasoning: Excellent refactoring that improves maintainability without breaking changes. Clean separation of concerns, proper layering, and zero API impact. The few minor type safety issues don't affect functionality and can be addressed in future iterations.

This PR successfully achieves its goal of splitting a large file into focused modules while maintaining perfect backward compatibility.

---

[greptile-apps]

Greptile Summary

This PR splits the 721-SLOC impact.ts monolith into three focused modules — fn-impact.ts (BFS traversal + function-level impact), diff-impact.ts (git diff parsing + diff-based impact), and presentation/diff-impact-mermaid.ts (Mermaid rendering) — while keeping impact.ts as a thin backward-compatible barrel. The split is a faithful code movement with no logic changes, and the prior architecture concern about a domain→presentation dependency edge has been resolved by re-exporting diffImpactMermaid from domain/queries.ts (pointing directly at presentation/) rather than from a domain analysis module.

Key observations:

• All 15 existing importers of impact.ts continue to work without path changes; bfsTransitiveCallers, fnImpactData, impactAnalysisData, and diffImpactData are all re-exported from the barrel.
• External callers in src/features/audit.ts and src/features/check.ts import bfsTransitiveCallers from domain/analysis/impact.js, which still resolves correctly through the new barrel.
• The new presentation/diff-impact-mermaid.ts correctly flows presentation → domain (importing diffImpactData from diff-impact.ts), which is the right layering direction.
• The outstanding CI / codegraph check --cycles --boundaries items in the test plan should be verified before merge to confirm no new graph cycles were introduced and that the boundary checker accepts queries.ts as a valid cross-layer barrel.

Confidence Score: 4/5

Safe to merge once CI and the boundary/cycle checks from the test plan are confirmed green.

The refactoring is a faithful, mechanical extraction with no logic changes. Backward compatibility is maintained through barrel re-exports, and the domain→presentation concern from the prior round has been addressed. The one remaining gate is CI validation (lint, build, tests, boundary check), which the author has listed as outstanding — that is a process gate, not a code defect.

No files require special attention beyond CI confirmation; src/domain/queries.ts is worth a quick look if the boundary checker flags the presentation import, but the intent is architecturally defensible.

Important Files Changed

Filename	Overview
src/domain/analysis/impact.ts	Reduced from 721 SLOC to a 2-line barrel re-exporting all public symbols from fn-impact.ts and diff-impact.ts; backward compat fully preserved.
src/domain/analysis/fn-impact.ts	New module containing bfsTransitiveCallers, impactAnalysisData, and fnImpactData; faithful extraction from the original impact.ts with no logic changes.
src/domain/analysis/diff-impact.ts	New module holding diffImpactData and all git-diff helpers; faithful extraction with correct cross-module import of bfsTransitiveCallers from fn-impact.ts.
src/domain/queries.ts	Removed diffImpactMermaid from the domain/analysis/impact.js re-export block and added a direct re-export from presentation/diff-impact-mermaid.js; resolves the domain→presentation concern raised in prior review.
src/presentation/diff-impact-mermaid.ts	New presentation-layer module for Mermaid diagram generation; faithfully extracted from impact.ts with the correct dependency direction (presentation → domain).

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    queries["src/domain/queries.ts\n(barrel re-export)"]
    impact["src/domain/analysis/impact.ts\n(barrel re-export)"]
    fnImpact["src/domain/analysis/fn-impact.ts\nbfsTransitiveCallers\nimpactAnalysisData\nfnImpactData"]
    diffImpact["src/domain/analysis/diff-impact.ts\ndiffImpactData\n+ git diff helpers"]
    mermaid["src/presentation/diff-impact-mermaid.ts\ndiffImpactMermaid"]

    queries -->|re-exports diffImpactData, fnImpactData, impactAnalysisData| impact
    queries -->|re-exports diffImpactMermaid| mermaid
    impact -->|re-exports| fnImpact
    impact -->|re-exports| diffImpact
    diffImpact -->|calls bfsTransitiveCallers| fnImpact
    mermaid -->|calls diffImpactData| diffImpact

    style queries fill:#e3f2fd,stroke:#1565c0
    style impact fill:#e8f5e9,stroke:#2e7d32
    style fnImpact fill:#f3e5f5,stroke:#6a1b9a
    style diffImpact fill:#fff3e0,stroke:#e65100
    style mermaid fill:#fce4ec,stroke:#880e4f

Loading

_{Reviews (2): Last reviewed commit: "fix: remove domain→presentation re-expor..." | Re-trigger Greptile}

[greptile-apps]

Domain barrel re-exports from presentation layer

impact.ts lives in domain/analysis/ but re-exports diffImpactMermaid from ../../presentation/diff-impact-mermaid.js. This introduces a domain → presentation dependency edge in the graph.

The project's layered architecture runs codegraph check --boundaries to enforce that domain code does not import presentation code, and the PR itself lists that check as an outstanding to-do. Any importer that currently does import { diffImpactMermaid } from './impact.js' will silently acquire this upward dependency, and the boundary check should flag it.

The safest fix for backward compat is to drop the diffImpactMermaid re-export from the domain barrel and let the 15 importers be updated to point directly at presentation/diff-impact-mermaid.js. Alternatively, keep the re-export but move impact.ts itself outside the domain/ subtree (e.g., as a top-level or features/ barrel), so it is no longer classified as domain code.

Suggested change

	*
	export { diffImpactData } from './diff-impact.js';
	export { bfsTransitiveCallers, fnImpactData, impactAnalysisData } from './fn-impact.js';

[carlos-alm]

Fixed. Removed the diffImpactMermaid re-export from domain/analysis/impact.ts so the domain barrel no longer crosses into the presentation layer. The re-export now lives in domain/queries.ts where it points directly at presentation/diff-impact-mermaid.js — queries.ts is a cross-cutting barrel so this is architecturally appropriate. All existing importers continue to work without changes.

[carlos-alm]

Fixed. Removed the diffImpactMermaid re-export from domain/analysis/impact.ts so the domain barrel no longer crosses into the presentation layer. The re-export now lives in domain/queries.ts where it points directly at presentation/diff-impact-mermaid.js — queries.ts is a cross-cutting barrel so this is architecturally appropriate. All existing importers continue to work without changes.

[carlos-alm]

This was already addressed in commit 15efa99, and the same split has since been independently merged to main via PRs #621 and #623. The domain barrel no longer re-exports diffImpactMermaid — it lives in domain/queries.ts pointing directly at presentation/diff-impact-mermaid.js.

[carlos-alm]

@greptileai

[carlos-alm]

Review Status

This PR's core changes (splitting impact.ts into fn-impact.ts, diff-impact.ts, and presentation/diff-impact-mermaid.ts) have already been independently merged to main via #623 (feat(titan): first full pipeline run), with additional lint cleanup in #621. The branch is now behind main with merge conflicts in both diff-impact.ts and impact.ts.

Review comments addressed:

• Greptile's P1 (domain barrel re-exports from presentation layer) — was already fixed in commit 15efa99 and the same fix exists on main.
• Claude bot review — approved with minor non-blocking suggestions (null checks, JSDoc).
• Greptile summary — confidence 4/5, gate was CI validation.

CI status: Lint failing due to pre-existing noNonNullAssertion warnings in the PR branch's older file versions. Main has these fixed.

Recommendation: Close this PR as superseded — the identical refactoring already exists on main. No unique changes remain that aren't already merged.

[carlos-alm]

Closing as superseded — the impact.ts split (fn-impact.ts + diff-impact.ts) already landed on main via #621 and #623. This branch now has unresolvable conflicts with the identical code already merged.