refactor: address warn-level complexity across features, visitors, presentation, and infrastructure#1593
Conversation
… DEFAULTS docs check acknowledged
…lt -- docs check acknowledged
…ocs check acknowledged
…odel -- docs check acknowledged
Codegraph Impact Analysis95 functions changed → 149 callers affected across 73 files
|
Greptile SummaryThis PR addresses warn-level cyclomatic and cognitive complexity violations across five domains by extracting helper functions — no algorithmic or behavioral changes are introduced. All extractions are faithful lifts of existing inline logic into named private functions with matching doc comments.
Confidence Score: 5/5Safe to merge — all changes are mechanical extractions of existing inline logic into named helpers with no algorithmic differences. Every extraction was verified against the original: loop variable re-assignment in No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "chore: merge origin/main — resolve confi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /** | ||
| * Return the latest version from cache if fresh, or fetch it from the registry. | ||
| * Persists the result to the cache before returning. | ||
| * Returns null when the network fetch fails or the cache is corrupt. | ||
| */ |
There was a problem hiding this comment.
The second bullet — "Persists the result to the cache before returning" — is inaccurate for the fresh-cache code path: when Date.now() - cache.lastCheckedAt < CACHE_TTL_MS, the function returns cache.latestVersion without calling saveCache. Persistence only happens on the stale/missing branch. The JSDoc should clarify that persistence is conditional on a network fetch, otherwise callers may assume every call refreshes the timestamp.
There was a problem hiding this comment.
Fixed — updated the JSDoc to accurately describe conditional persistence: the cache is only written when a network fetch occurs (stale/missing cache path). When the cache is fresh, the value is returned directly without saving.
| function deserializeResult(ser: SerializedExtractorOutput | null): ExtractorOutput | null { | ||
| if (!ser) return null; | ||
| const out: ExtractorOutput = { ...deserializeCoreFields(ser) } as ExtractorOutput; | ||
| deserializeBindingFields(ser, out); | ||
| deserializeMapFields(ser, out); | ||
| return out; | ||
| } |
There was a problem hiding this comment.
ExtractorOutput
deserializeCoreFields returns Pick<ExtractorOutput, …> & Partial<Pick<ExtractorOutput, …>>, which intentionally omits the binding and Map fields. The spread-then-cast { ...deserializeCoreFields(ser) } as ExtractorOutput silences TypeScript for the gap between what the core helper returns and the full ExtractorOutput shape. If a new required field is added to ExtractorOutput and assigned in one of the three helpers but forgotten in another, the compiler will not catch the partial construction here.
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.
Added an inline comment explaining why the cast is safe and documenting the maintenance contract: all required ExtractorOutput fields are covered by deserializeCoreFields's Pick return type. The comment instructs future devs to update deserializeCoreFields's return type and body when adding new required fields to ExtractorOutput.
Keep main's improved CODEGRAPH_ENGINE validation (with explicit warn on invalid values) and the updated engine JSDoc that documents partial wiring.
|
Addressed both Greptile P2 findings and resolved merge conflicts with main: Conflicts resolved (src/infrastructure/config.ts, src/types.ts): Kept main's improved CODEGRAPH_ENGINE validation (explicit warn on invalid values) and the updated engine JSDoc documenting partial wiring (issue #1596). Finding 1 (update-check.ts:126): Updated JSDoc to accurately state that persistence is conditional — the cache is only written when a network fetch occurs (stale/missing path). The cache-hit path returns directly without saving. Finding 2 (wasm-worker-pool.ts:151): Added an inline comment documenting the maintenance contract for the force-cast: all required ExtractorOutput fields are covered by deserializeCoreFields's Pick return type, and the comment instructs future devs to keep deserializeCoreFields in sync when adding new required fields. |
1 participant
Summary
Addresses warn-level complexity violations across 5 domains that cleared the FAIL threshold but remained above WARN thresholds. Grouped as a single PR since all are below-FAIL cleanup rather than structural decompositions.
presentation/viewer.ts—renderPlotHTMLdecomposed; MI improveddomain/wasm-worker-pool.ts—deserializeResultextractedfeatures/(8 files: audit, boundaries, cfg, check, communities, sequence, snapshot, triage) — warn-level complexity addressed, empty catch infeatures/structure.tsfixedast-analysis/visitors/(complexity-visitor, dataflow-visitor, visitor.ts) — visitor dispatch helpers extractedinfrastructure/registry.ts,update-check.ts—checkForUpdatescyclomatic: 13→10graph/algorithms/tarjan.ts— complexity addressedgraph/builders/dependency.ts— complexity addressedshared/normalize.ts— complexity addressedTitan Audit Context
Metrics Impact
checkForUpdatescyclomatic: 13→10features/structure.tsfixed (Rule II.10 violation resolved)Test plan
codegraph check --cycles --boundariespasses