refactor: address SLOC warnings in domain and features layers#621
refactor: address SLOC warnings in domain and features layers#621carlos-alm merged 9 commits intomainfrom
Conversation
…d domain layers Impact: 8 functions changed, 33 affected
Impact: 2 functions changed, 5 affected
|
Claude finished @carlos-alm's task —— View job Root Cause AnalysisProblem Being Solved: Addressing SLOC (Source Lines of Code) warnings from Titan audit Phase 5 by extracting query logic from large files ( Backlog Compliance
Code Quality AssessmentArchitectural Soundness ⭐⭐⭐⭐
Implementation Quality ⭐⭐⭐⭐
Dependency Management ⭐⭐⭐⭐⭐
Minor Concerns
Final Recommendation
Recommendation: Approve and merge. This refactoring will make the codebase more maintainable and easier for AI agents to navigate through clearer module boundaries. |
Greptile SummaryThis PR is a follow-up cleanup that addresses two previously flagged issues: the Confidence Score: 5/5Safe to merge — all previously flagged issues have been resolved and no new issues are introduced. Both prior review concerns (domain→presentation layer inversion and missing 'files' sort case) are cleanly addressed. The changes are small, targeted, and consistent with the surrounding code patterns. No regressions or new logic risks are present. No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "fix: suppress noExplicitAny lint error i..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/domain/analysis/impact.ts
Outdated
|
|
||
| return lines.join('\n'); | ||
| } | ||
| export { diffImpactMermaid } from '../../presentation/diff-impact-mermaid.js'; |
There was a problem hiding this comment.
impact.ts now re-exports diffImpactMermaid from ../../presentation/diff-impact-mermaid.js, making a domain/analysis module depend on a presentation module. Per the architecture in CLAUDE.md, presentation/ is the output-formatting layer and should depend on domain/, not the reverse.
The move of diffImpactMermaid into presentation/ is architecturally correct; the backward-compat re-export here is what creates the inversion. If all callers of diffImpactMermaid already live in the presentation/ layer (or in CLI entry points), this re-export may not be needed and can be removed to keep the boundary clean. If some domain-layer consumers do exist, consider whether they should instead import directly from the new canonical location (presentation/diff-impact-mermaid.ts).
There was a problem hiding this comment.
Fixed in ca30a5b. Removed the diffImpactMermaid re-export from the domain barrel (domain/analysis/impact.ts). Instead, domain/queries.ts now re-exports it directly from presentation/diff-impact-mermaid.ts, which keeps the dependency direction correct (queries.ts is consumed by presentation/MCP callers, not by domain-layer code).
| // Get files inside this directory | ||
| const files = ( | ||
| db | ||
| .prepare(` | ||
| SELECT n.name FROM edges e | ||
| JOIN nodes n ON e.target_id = n.id | ||
| WHERE e.source_id = ? AND e.kind = 'contains' AND n.kind = 'file' | ||
| `) | ||
| .all(d.id) as { name: string }[] | ||
| ).map((f) => f.name); | ||
|
|
||
| return { | ||
| directory: d.name, | ||
| cohesion: d.cohesion, |
There was a problem hiding this comment.
'files' case in getSortFn — default sort is silently alphabetical
structureData defaults sortBy to 'files' when opts.sort is not provided (const sortBy = opts.sort || 'files'), but getSortFn has no 'files' case, so it falls through to a.name.localeCompare(b.name) (alphabetical by directory name).
This was pre-existing behavior in the original structure.ts, so this PR does not regress it — but since the function is being extracted here, it's a good moment to fix the intent or align the default:
| // Get files inside this directory | |
| const files = ( | |
| db | |
| .prepare(` | |
| SELECT n.name FROM edges e | |
| JOIN nodes n ON e.target_id = n.id | |
| WHERE e.source_id = ? AND e.kind = 'contains' AND n.kind = 'file' | |
| `) | |
| .all(d.id) as { name: string }[] | |
| ).map((f) => f.name); | |
| return { | |
| directory: d.name, | |
| cohesion: d.cohesion, | |
| default: | |
| // 'files' and any unknown sort key: fall back to file-count descending | |
| return (a, b) => (b.file_count || 0) - (a.file_count || 0); |
Or align the default sortBy value with the actual behavior:
const sortBy = opts.sort || 'name';There was a problem hiding this comment.
Fixed in a66ab96. Added an explicit \ case to \ that sorts by \ descending, which aligns with the intent of the default \ value. The alphabetical fallback in \ now only applies to truly unknown sort keys.
Remove the diffImpactMermaid re-export from domain/analysis/impact.ts (which created a domain→presentation dependency inversion). Instead, re-export it directly from domain/queries.ts → presentation/diff-impact-mermaid.ts. All callers already consume through domain/queries.ts, so this is transparent.
The default sortBy value is 'files' but getSortFn had no matching case, causing it to silently fall through to alphabetical sort. Add explicit 'files' case that sorts by file_count descending. Impact: 1 functions changed, 4 affected
# Conflicts: # src/domain/analysis/diff-impact.ts # src/domain/analysis/impact.ts # src/features/structure-query.ts
1 similar comment
1 participant
Summary
complexity.tsandstructure.tsto extract query-building logic into dedicated modulescomplexity-query.ts,structure-query.tsTitan Audit Context
Changes
src/features/complexity.ts-- query logic extracted to complexity-query.tssrc/features/complexity-query.ts-- new: complexity data querying and formattingsrc/features/structure.ts-- query logic extracted to structure-query.tssrc/features/structure-query.ts-- new: structure data querying and formattingMetrics Impact
Test plan