refactor: Titan v3.8.0 — decompose god-functions, structured logging, error handling#775
refactor: Titan v3.8.0 — decompose god-functions, structured logging, error handling#775carlos-alm merged 29 commits intomainfrom
Conversation
Replace catch blocks that silently swallow errors with proper error variable capture and debug() logging across 10 files. This addresses Pillar II (Rule 10: emptyCatch) violations identified by the gauntlet audit. Files that already had proper error handling were verified and left unchanged.
Decompose walkWithVisitors (cognitive=65, halstead.bugs=1.05) into focused helper functions: mergeFunctionNodeTypes, initVisitors, isSkipped, dispatchEnterFunction, dispatchEnterNode, dispatchExitNode, clearSkipFlags, dispatchExitFunction, collectResults. The main function is now a thin orchestrator delegating to single-purpose dispatch helpers.
…r-utils Split extractParamNames by extracting extractRestParamNames, extractObjectDestructNames, and extractArrayDestructNames. Extract resolveOptionalChainCallee from resolveCalleeName. Reduces cognitive complexity and cyclomatic complexity of the two main functions.
Extract resolveValueNode, unwrapExpressionList, recordDestructuredAssignment, and recordSimpleAssignment from the monolithic handleVarDeclarator function. Reduces cognitive complexity from 48 to focused single-purpose helpers.
Consolidate the enterNode classification logic into a single classifyNode function. Simplify enterFunction/exitFunction by merging equivalent branches for fileLevelWalk mode. Reduces cognitive complexity from 37 to focused single-purpose helpers.
…tion Extract extractThrowName, extractAwaitName from extractName (removed). Extract resolveNameAndText from collectNode to separate per-kind name/text resolution from the row-building logic. Reduces cognitive complexity and cyclomatic branching in the visitor factory.
…ride Extract detectNativeNeeds and runNativeFileAnalysis from runNativeAnalysis to separate needs-detection from per-file dispatch. Extract shared overrideCyclomaticFromCfg helper to eliminate duplicated MI recalculation in storeNativeCfgResults and storeCfgResults.
Replace magic numbers in the MI formula with named constants (MI_BASE, MI_VOLUME_COEFF, etc.) and the Halstead bugs divisor. Extract sumCounts helper from computeHalsteadDerived. Reduces halsteadEffort and addresses Rule 7 magic-values warning.
Extract repeated patterns into focused helper functions: - pipeline.ts: closeNativeDb, reopenNativeDb, suspendNativeDb, refreshJsDb - insert-nodes.ts: marshalSymbolBatches, buildFileHashes - resolve-imports.ts: buildReexportMap, findBarrelCandidates, reparseBarrelFiles - watcher.ts: prepareWatcherStatements, processPendingFiles, writeJournalAndChangeEvents
Extract helper functions from main handler functions in solidity, zig, javascript, python, go, rust, and java extractors to reduce cognitive complexity, cyclomatic complexity, and halstead effort metrics.
Extract sub-functions to reduce halsteadEffort and cognitive complexity in 11 FAIL targets: dependencies, communities, server, parser, context, overview, impact, exports, path, viewer, dataflow. All functions now pass gate thresholds (cognitive <= 30, halstead.bugs <= 1.0, MI >= 20).
|
Claude finished @carlos-alm's task —— View job Pull Request Review - COMPREHENSIVE ASSESSMENTReviewing this large refactor PR (57 files, +3665/-2647) following the 5-phase review protocol: Task List
Root Cause AnalysisProblem Being Solved: Systematic technical debt remediation identified by automated Titan v3.8.0 audit targeting 20 highest-risk symbols across 13 domains. This addresses real quality issues, not arbitrary refactoring. Why This Approach: Evidence-based refactoring with concrete metrics improvement. Community drift score improved 40→28, min maintainability index 13.4→20.3. Risk Assessment: ✅ Low Risk - Addresses genuine technical debt with measurable improvements. Backlog Compliance
Critical ConcernsNone Identified. This PR demonstrates exemplary refactoring practices: ✅ Strengths
✅ No Red Flags
✅ Code Quality Exemplars
✅ Process Alignment
Final Recommendation
Minor Observations
Verdict: This PR represents the gold standard for systematic refactoring. Approve with confidence. |
Greptile SummaryThis PR is a broad Titan v3.8.0 refactoring pass: god-functions are decomposed into focused helpers, empty Confidence Score: 5/5This PR is safe to merge — all refactors are behaviorally equivalent to the code they replace, prior review concerns are resolved, and no new P0/P1 issues were found. The prior P1 concern (shared→infrastructure layer inversion) is fully resolved by suppress.ts. All decompositions were traced through carefully: partition.ts closures → PartitionState preserves array identity; complexity.ts handlers match original control-flow; ast.ts suspend/resume WAL guard is intact; pipeline.ts NativeDb lifecycle is equivalent. All remaining findings would be P2 at most. src/graph/algorithms/leiden/partition.ts and src/features/complexity.ts deserve spot-check testing since they implement numerically sensitive algorithms, but the logic traces correctly. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[buildAstNodes] --> B{tryNativeBulkInsert}
B -->|true| C[return — native handled all nodes]
B -->|false| D[JS fallback: collectFileAstRows per file]
D --> E[bulk INSERT into ast_nodes table]
F[runPipelineStages] --> G[suspendNativeDb]
G --> H[collectFiles]
H --> I[reopenNativeDb for insertNodes]
I --> J[insertNodes]
J --> K[closeNativeDb + refreshJsDb]
K --> L[resolveImports / buildEdges]
L --> M[reopenNativeDb for analyses]
M --> N[run analyses]
N --> O[closeNativeDb post-analyses]
O --> P[finalize]
Q[makePartition] --> R[PartitionState struct]
R --> S[ensureCommCapacity]
R --> T[initAggregates]
R --> U[accumulateNeighborWeights]
R --> V[moveNode]
R --> W[compactIds]
X[refineWithinCoarseCommunities] --> Y[collectRefinementCandidates]
Y --> Z[boltzmannSelectCandidate]
Z -->|chosenC >= 0| AA[moveNodeToCommunity]
Z -->|chosenC == -1| AB[node stays as singleton]
Reviews (2): Last reviewed commit: "fix: correct TypeScript type errors in e..." | Re-trigger Greptile |
| * Library code throws these instead of calling process.exit() or throwing | ||
| * bare Error instances. The CLI top-level catch formats them for humans; | ||
| * MCP returns structured { isError, code } responses. | ||
| */ |
There was a problem hiding this comment.
shared/ importing from infrastructure/ crosses layer boundary
shared/errors.ts was previously dependency-free — a pure domain module. Adding import { debug } from '../infrastructure/logger.js' makes the shared/ layer depend on infrastructure/, inverting the expected dependency direction (infrastructure should depend on shared, not the other way around).
This affects anything that imports from shared/errors.ts (virtually the entire codebase), since it now transitively pulls in the logger. Consider keeping suppressError/suppressErrorAsync in a dedicated shared/catch-utils.ts that imports from the logger, or moving them into infrastructure/ directly, leaving errors.ts import-free.
There was a problem hiding this comment.
Fixed — moved suppressError and suppressErrorAsync to src/infrastructure/suppress.ts. The shared/errors.ts module is now dependency-free again (no infrastructure imports). A doc comment in errors.ts points to the new location.
src/db/query-builder.ts
Outdated
| /** Track parenthesis depth change for a character. */ | ||
| function parenDepthDelta(ch: string): number { | ||
| if (ch === '(') return 1; | ||
| if (ch === ')') return -1; | ||
| return 0; | ||
| } | ||
|
|
||
| function splitTopLevelCommas(str: string): string[] { | ||
| const parts: string[] = []; | ||
| let depth = 0; | ||
| let start = 0; | ||
| for (let i = 0; i < str.length; i++) { | ||
| if (str[i] === '(') depth++; | ||
| else if (str[i] === ')') depth--; | ||
| else if (str[i] === ',' && depth === 0) { | ||
| depth += parenDepthDelta(str[i]); | ||
| if (str[i] === ',' && depth === 0) { |
There was a problem hiding this comment.
parenDepthDelta changes comma-split evaluation order
In the original, the three cases were mutually exclusive via if/else if:
if (str[i] === '(') depth++;
else if (str[i] === ')') depth--;
else if (str[i] === ',' && depth === 0) { ... }The new code first applies parenDepthDelta then unconditionally checks the comma condition. The behavior is identical because a character cannot simultaneously be '(' and ',', but the logic is now slightly harder to reason about. A brief comment would prevent future confusion.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed — added a clarifying JSDoc comment to parenDepthDelta explaining that ( and ) can never simultaneously be ,, so the comma check in the caller remains mutually exclusive with the depth update. Also extracted str[i] into a local ch variable to satisfy noUncheckedIndexedAccess.
Keep decomposed helper functions from Titan refactor while incorporating the normalizePath() fix for Windows batch-resolved key lookups from #758.
# Conflicts: # src/domain/analysis/dependencies.ts # src/shared/normalize.ts
Resolves layer-boundary violation where shared/errors.ts imported from infrastructure/logger.ts. The suppress helpers now live in infrastructure/ alongside their logger dependency, keeping shared/ dependency-free.
Type Map<unknown, unknown> in buildNodeDepsResult to Map<string, string | null>. Use NativeDatabase type in buildNativeDataflowResult parameter.
Summary
Automated Titan Paradigm run targeting the 20 highest-risk symbols across 13 domains. Full pipeline: recon → gauntlet (69 targets audited) → sync → forge (14 execution phases, 63 targets completed, 0 failures).
Key changes:
console.login non-CLI code withlogger.warn/logger.debugmakePartition,hybridSearchData,buildAstNodes,walkWithVisitors, and multiple feature/presentation functions split into focused sub-functionsshared/normalize.ts,db/query-builder.ts,db/repository/nodes.ts,graph/model.tsMetrics delta:
Test plan
npm testpasses (core JS/TS/TSX tests — parser-availability failures are pre-existing)npm run lintpassescodegraph statscodegraph audit --quick src/to verify structural health