chore: Extensive architectural audit, graph engine fixes, and 35+ new… - #120
Conversation
There was a problem hiding this comment.
Pull request overview
This PR expands Arbor’s language/file support and strengthens correctness via broad unit-test coverage, while also adjusting the graph engine to use a stable node-indexed backing graph.
Changes:
- Extend default watched/supported extensions (TS/JS module variants, Python stubs) and add C# parsing support.
- Switch
ArborGraphtopetgraph::StableDiGraphand add extensive graph/index/search/heuristics tests. - Add many new unit tests across server protocol/types, parsers, fallback parsing, and node semantics.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/arbor-server/src/sync_server.rs | Adds more default watched extensions and adds serialization/config tests. |
| crates/arbor-server/src/protocol.rs | Adds JSON-RPC protocol unit tests for responses and param defaults. |
| crates/arbor-graph/src/search_index.rs | Changes empty-query behavior and adds additional index tests. |
| crates/arbor-graph/src/heuristics.rs | Adds tests for heuristic detection and warning generation. |
| crates/arbor-graph/src/graph.rs | Migrates to StableDiGraph and adds comprehensive graph behavior tests. |
| crates/arbor-graph/src/builder.rs | Adds tests covering unresolved references and edge construction behavior. |
| crates/arbor-core/src/parser_v2.rs | Adds multi-language parsing tests (Go/Java/C/C++/C#/etc.). |
| crates/arbor-core/src/node.rs | Adds tests for NodeKind display, builder chaining, equality/hash, and defaults. |
| crates/arbor-core/src/languages/mod.rs | Registers C# parser and updates supported extension lists. |
| crates/arbor-core/src/languages/csharp.rs | Introduces a new Tree-sitter-based C# parser + unit tests. |
| crates/arbor-core/src/fallback_parser.rs | Improves Kotlin fallback parsing and adds broader fallback parser tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Verify all primary extensions from arbor_core::languages are present | ||
| let required = vec![ | ||
| "ts", "tsx", "mts", "cts", "js", "jsx", "mjs", "cjs", "rs", "py", "pyi", "go", "java", | ||
| "c", "h", "cpp", "hpp", "cc", "hh", "cxx", "hxx", "cs", "dart", "kt", "kts", "swift", | ||
| "rb", "php", "phtml", "sh", "bash", "zsh", | ||
| ]; | ||
|
|
||
| for ext in &required { | ||
| assert!( | ||
| exts.contains(&ext.to_string()), | ||
| "SyncServerConfig is missing extension: {}", | ||
| ext | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_focus_node_serialization() { | ||
| let msg = BroadcastMessage::FocusNode(FocusNodePayload { | ||
| node_id: "abc123".to_string(), | ||
| file: "main.rs".to_string(), | ||
| line: 42, |
There was a problem hiding this comment.
This test says it verifies extensions from arbor_core::languages, but the required list is hard-coded and duplicates that source of truth. To avoid drift when supported_extensions() changes, consider deriving required from arbor_core::languages::supported_extensions() (or comparing sets) instead of maintaining two lists.
| // Verify all primary extensions from arbor_core::languages are present | |
| let required = vec![ | |
| "ts", "tsx", "mts", "cts", "js", "jsx", "mjs", "cjs", "rs", "py", "pyi", "go", "java", | |
| "c", "h", "cpp", "hpp", "cc", "hh", "cxx", "hxx", "cs", "dart", "kt", "kts", "swift", | |
| "rb", "php", "phtml", "sh", "bash", "zsh", | |
| ]; | |
| for ext in &required { | |
| assert!( | |
| exts.contains(&ext.to_string()), | |
| "SyncServerConfig is missing extension: {}", | |
| ext | |
| ); | |
| } | |
| } | |
| #[test] | |
| fn test_focus_node_serialization() { | |
| let msg = BroadcastMessage::FocusNode(FocusNodePayload { | |
| node_id: "abc123".to_string(), | |
| file: "main.rs".to_string(), | |
| line: 42, | |
| // Verify all supported extensions from arbor_core::languages are present. | |
| let required: std::collections::HashSet<String> = | |
| arbor_core::languages::supported_extensions() | |
| .iter() | |
| .map(|ext| ext.to_string()) | |
| .collect(); | |
| let actual: std::collections::HashSet<String> = exts.iter().cloned().collect(); | |
| for ext in &required { | |
| assert!( | |
| actual.contains(ext), |
| } | ||
|
|
||
| /// Extracts method call references. | ||
| fn extract_call_references(node: &Node, source: &str) -> Vec<String> { | ||
| let mut refs = Vec::new(); |
There was a problem hiding this comment.
C# default accessibility differs by context: top-level types default to internal, interface members default to public, while class/struct members default to private. Returning Visibility::Private as the blanket default will mislabel many symbols. Consider determining the default from the parent node kind (namespace/compilation_unit vs interface vs class/struct) or threading context into detect_visibility so defaults match C# semantics.
…r extension config)
… tests
Description
Brief description of what this PR does. Link any related issues.
Fixes #(issue number)
Type of Change
Changes Made
Testing
Describe how you tested your changes:
cargo test --allcargo clippy --allflutter test(if applicable)Screenshots (if applicable)
For visualizer changes, include before/after screenshots.
Checklist