refactor(native): mirror crate module layout to the src/ TypeScript tree - #1463
refactor(native): mirror crate module layout to the src/ TypeScript tree#1463carlos-alm merged 3 commits into
Conversation
Sorry, something went wrong.
Reorganize crates/codegraph-core/src/ so every module sits at the path of its TypeScript counterpart (snake_case for kebab-case): shared/, infrastructure/, db/repository/, domain/graph/builder/stages/, ast_analysis/, graph/algorithms/, graph/classifiers/, features/. - Pure git mv moves; only graph_algorithms.rs is split (bfs, shortest_path, centrality, louvain) along its existing section boundaries - lib.rs doc comment carries the full Rust<->TypeScript mapping table - Cross-references in TS sources, tests, and docs updated to new paths - Cargo.lock version synced to 3.12.0 (Cargo.toml was already bumped) - cargo test: 360 passed; tsc build and drift-guard test green
|
Claude finished @carlos-alm's task —— View job PR Review: refactor(native): mirror crate module layout to the src/ TypeScript treeTasks:
Root Cause AnalysisProblem Being Solved: Structural divergence between crates/codegraph-core/src/ and src/ made parity work expensive — equivalent logic lived at unrelated paths, requiring tribal knowledge to locate counterparts. The v3.12.0 dogfood report (#1455) surfaced this as a friction point blocking efficient parity gap detection. Why This Approach: Pure structural reorganization — git mv moves with no behavioral changes, plus one meaningful split of graph_algorithms.rs along existing section boundaries into discrete files (bfs.rs, centrality.rs, shortest_path.rs, louvain.rs). The module split is sound: the new files introduce fresh unit tests that weren't present before, and the public API is re-exported from algorithms/mod.rs identically. Risk Assessment: Low. All changes are:
Phase 2: Configuration Change DetectionNo lint rules relaxed, no test timeouts raised, no coverage thresholds lowered, no TypeScript strict flags modified. ✓ Clean. Phase 3: Test IntegrityAll test file changes are path-string updates only — no assertions weakened:
Phase 4: Code QualityPositives:
One minor concern: Extractor changes: All 30+ extractor files have identical 2-line use statement updates (e.g., helpers.rs import path update). Spot-checked javascript.rs, kotlin.rs, clojure.rs — changes are consistently updating use crate::ast_analysis::... paths after the module move. No logic changes. ✓ Backlog Compliance
Critical ConcernsNone. This is a clean structural refactor. The only finding worth noting is that native-orchestrator.ts:1305 now references collect_files (the new module name) instead of the old file_collector — this is a correct update because collect_files.rs is the actual filename after the rename. The comment now accurately describes the code. Final Recommendation
|
Sorry, something went wrong.
Greptile SummaryThis PR reorganizes crates/codegraph-core/src/ so each Rust module mirrors its TypeScript counterpart path (snake_case ↔ kebab-case), motivated by structural divergence that made parity gaps hard to spot in the v3.12.0 dogfood report.
Confidence Score: 5/5Safe to merge — all 360 cargo tests and the tsc build pass; changes are structural moves with no logic modifications. This is a pure module reorganization: files were moved, use paths updated, and comments/cross-references refreshed. The algorithm split of graph_algorithms.rs follows existing section boundaries with no behavioral changes. Two integration-test comments (file_collector / edge_builder) were missed in the reference sweep but have no runtime impact. No files require special attention; the two stale comments in tests/integration/dropped-language-gap.test.ts and tests/integration/this-dispatch-scope.test.ts are cosmetic and do not affect correctness. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph OLD["Before (flat layout)"]
O1[parser_registry.rs]
O2[native_db.rs]
O3[analysis.rs]
O4[graph_algorithms.rs]
O5[cycles.rs]
O6[roles_db.rs]
O7[ast_db.rs / edges_db.rs]
end
subgraph NEW["After (mirrored layout)"]
N1[domain/parser.rs]
N2[db/connection.rs]
N3[ast_analysis/engine.rs]
subgraph ALG["graph/algorithms/"]
A1[bfs.rs]
A2[centrality.rs]
A3[louvain.rs]
A4[shortest_path.rs]
A5[tarjan.rs]
end
N6[graph/classifiers/roles.rs]
N7[db/repository/ast.rs / edges.rs]
end
O1 -->|mv| N1
O2 -->|mv| N2
O3 -->|mv| N3
O4 -->|split| ALG
O5 -->|mv| A5
O6 -->|mv| N6
O7 -->|mv| N7
Reviews (4): Last reviewed commit: "Merge branch 'main' into refactor/native..." | Re-trigger Greptile |
Sorry, something went wrong.
| pub use bfs::{bfs_traversal, BfsEntry}; | ||
| pub use centrality::{fan_in_out, FanInOutEntry}; | ||
| pub use louvain::{louvain_communities, CommunityAssignment, LouvainResult}; | ||
| pub use shortest_path::shortest_path; |
There was a problem hiding this comment.
tarjan::detect_cycles is the only algorithm in this module not re-exported via pub use, even though every other public function (bfs_traversal, fan_in_out, louvain_communities, shortest_path) is. This forces callers to write the full three-segment path graph::algorithms::tarjan::detect_cycles while everything else is at graph::algorithms::*. A pub use tarjan::detect_cycles here keeps the surface consistent and makes future call-site refactors uniform.
| pub use bfs::{bfs_traversal, BfsEntry}; | |
| pub use centrality::{fan_in_out, FanInOutEntry}; | |
| pub use louvain::{louvain_communities, CommunityAssignment, LouvainResult}; | |
| pub use shortest_path::shortest_path; | |
| pub use bfs::{bfs_traversal, BfsEntry}; | |
| pub use centrality::{fan_in_out, FanInOutEntry}; | |
| pub use louvain::{louvain_communities, CommunityAssignment, LouvainResult}; | |
| pub use shortest_path::shortest_path; | |
| pub use tarjan::detect_cycles; |
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!
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed — added pub use tarjan::detect_cycles; to algorithms/mod.rs so the re-export surface is now consistent: all five algorithm entry points (bfs_traversal, fan_in_out, louvain_communities, shortest_path, detect_cycles) are re-exported at graph::algorithms::*.
Sorry, something went wrong.
| #[napi] | ||
| pub fn bfs_traversal( | ||
| edges: Vec<GraphEdge>, | ||
| start_ids: Vec<String>, | ||
| max_depth: Option<i32>, | ||
| direction: Option<String>, | ||
| ) -> Vec<BfsEntry> { | ||
| let graph = DirectedGraph::from_edges(&edges); | ||
| let max_depth = max_depth.unwrap_or(i32::MAX); |
There was a problem hiding this comment.
Using i32::MAX as the "no limit" sentinel means depths are a signed type, allowing the depth + 1 expression to overflow when a graph path exceeds ~2 billion hops. In debug builds Rust panics on overflow; in release it silently wraps to i32::MIN, and the check depth >= max_depth (i32::MIN >= i32::MAX) becomes false, letting BFS continue past the intended cap. Switching to u32 (depths are never negative) and defaulting the absent cap to u32::MAX removes both the signed-semantics oddity and the overflow window.
| #[napi] | |
| pub fn bfs_traversal( | |
| edges: Vec<GraphEdge>, | |
| start_ids: Vec<String>, | |
| max_depth: Option<i32>, | |
| direction: Option<String>, | |
| ) -> Vec<BfsEntry> { | |
| let graph = DirectedGraph::from_edges(&edges); | |
| let max_depth = max_depth.unwrap_or(i32::MAX); | |
| #[napi] | |
| pub fn bfs_traversal( | |
| edges: Vec<GraphEdge>, | |
| start_ids: Vec<String>, | |
| max_depth: Option<u32>, | |
| direction: Option<String>, | |
| ) -> Vec<BfsEntry> { | |
| let graph = DirectedGraph::from_edges(&edges); | |
| let max_depth = max_depth.unwrap_or(u32::MAX); |
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed — changed max_depth from Option<i32> to Option<u32> and the internal depth tracking from HashMap<&str, i32> to HashMap<&str, u32>, with u32::MAX as the absent-cap sentinel. The BfsEntry.depth field stays i32 (cast at collection time) since that's the napi boundary type — both map identically to TypeScript number, so the generated .d.ts is unchanged.
Sorry, something went wrong.
Codegraph Impact Analysis59 functions changed → 45 callers affected across 14 files
|
Sorry, something went wrong.
- bfs.rs: change max_depth from Option<i32> to Option<u32> and use u32 throughout the BFS depth tracking to eliminate the i32::MAX overflow sentinel and wrap-to-i32::MIN risk in release builds - mod.rs: add pub use tarjan::detect_cycles so the re-export surface is consistent with every other algorithm function - tarjan.rs: fix stale doc comment path from src/cycles.js to the correct src/graph/algorithms/tarjan.ts counterpart
|
Addressed Greptile's review feedback:
|
Sorry, something went wrong.
Sorry, something went wrong.
Summary
Motivation
The v3.12.0 dogfood report (#1455) showed that structural divergence between /src and /crates/codegraph-core makes parity gaps hard to spot and fix: equivalent logic lives at unrelated paths, so side-by-side comparison requires tribal knowledge. With mirrored layouts, every resolver/extractor/builder file has an obvious counterpart, which the follow-up parity work builds on.
Test plan