Skip to content

Commit a765489

Browse files
committed
refactor: split major code smell hotspots
1 parent 49f1961 commit a765489

24 files changed

Lines changed: 7182 additions & 6351 deletions
Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
# Code Smell Remediation Implementation Plan
2+
3+
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
4+
5+
**Goal:** Reduce the highest-maintenance code smells in Grapha without changing user-visible behavior, starting with the broken Swift index-store config and then splitting the largest multi-responsibility modules into focused units.
6+
7+
**Architecture:** Fix correctness first by replacing the dead process-global index-store toggle with explicit configuration flow through the extraction pipeline. Then perform bounded structural refactors in three areas: Swift tree-sitter enrichment, CLI orchestration, and SQLite persistence. Each phase should preserve behavior with targeted tests before any further decomposition.
8+
9+
**Tech Stack:** Rust workspace, clap, rusqlite, rayon, existing `grapha_core` plugin/extraction pipeline, existing integration/unit tests with `assert_cmd`
10+
11+
---
12+
13+
## File Structure
14+
15+
| File | Responsibility |
16+
|------|----------------|
17+
| Modify: `grapha/src/main.rs` | Current CLI entrypoint, pipeline orchestration, command dispatch, watch-mode server path |
18+
| Modify: `grapha/src/config.rs` | `swift.index_store` configuration source of truth |
19+
| Modify: `grapha-swift/src/lib.rs` | Swift extraction waterfall and index-store discovery |
20+
| Modify: `grapha-swift/src/treesitter.rs` | Current tree-sitter extractor plus all Swift enrichment passes |
21+
| Modify: `grapha/src/store/sqlite.rs` | Current SQLite schema, save/load, incremental sync, compatibility decode |
22+
| Create: `grapha-swift/src/treesitter/` | New focused Swift tree-sitter extraction/enrichment modules |
23+
| Create: `grapha/src/app/` or `grapha/src/commands/` | Extracted CLI orchestration and command handlers |
24+
| Create: `grapha/src/store/sqlite/` | Extracted SQLite schema/read/write helpers |
25+
| Modify: `grapha/tests/integration.rs` | CLI behavior regression coverage |
26+
| Modify: `grapha-swift/tests/*.rs` | Swift extraction/config regression coverage |
27+
28+
---
29+
30+
### Task 1: Fix the dead Swift index-store toggle
31+
32+
**Files:**
33+
- Modify: `grapha/src/main.rs`
34+
- Modify: `grapha/src/config.rs`
35+
- Modify: `grapha-swift/src/lib.rs`
36+
- Modify: `grapha-core` extraction context files if pipeline options must be carried through shared context types
37+
- Test: existing Swift extraction tests or new targeted regression coverage in `grapha-swift/tests/`
38+
39+
- [ ] Add a failing regression test that proves `[swift].index_store = false` prevents index-store extraction and falls back to SwiftSyntax/tree-sitter behavior.
40+
- [ ] Remove the `unsafe { std::env::set_var("GRAPHA_SKIP_INDEX_STORE", "1") }` branch from [main.rs](/Users/wendell/Developer/oops-rs/grapha/grapha/src/main.rs#L484) and replace it with explicit pipeline configuration.
41+
- [ ] Thread an `index_store_enabled` flag through the pipeline into `grapha_swift::extract_swift(...)` so the behavior is controlled by typed inputs rather than process-global state.
42+
- [ ] Update `grapha-swift/src/lib.rs` so `extract_swift` only initializes/uses index store when the flag is enabled.
43+
- [ ] Run targeted Swift tests, then `cargo test -p grapha-swift` and `cargo test -p grapha -- integration`.
44+
- [ ] Commit the fix separately before any structural refactor work.
45+
46+
### Task 2: Split Swift tree-sitter extraction from enrichment passes
47+
48+
**Files:**
49+
- Modify: `grapha-swift/src/treesitter.rs`
50+
- Create: `grapha-swift/src/treesitter/mod.rs`
51+
- Create: `grapha-swift/src/treesitter/extract.rs`
52+
- Create: `grapha-swift/src/treesitter/doc_comments.rs`
53+
- Create: `grapha-swift/src/treesitter/swiftui.rs`
54+
- Create: `grapha-swift/src/treesitter/localization.rs`
55+
- Create: `grapha-swift/src/treesitter/assets.rs`
56+
- Create: `grapha-swift/src/treesitter/common.rs` if shared span/index helpers remain cross-cutting
57+
- Test: existing Swift tree-sitter tests plus new module-focused unit tests
58+
59+
- [ ] Freeze behavior first with targeted tests around doc-comment enrichment, SwiftUI structure enrichment, localization metadata, and asset reference tagging.
60+
- [ ] Keep a thin public facade at `grapha-swift/src/treesitter.rs` or `mod.rs` that re-exports the current public API so call sites stay stable during the split.
61+
- [ ] Move generic parser/shared types (`parse_swift`, `EnrichmentContext`, span helpers, dedup helpers) into `common`/`extract` modules.
62+
- [ ] Move each enrichment concern into its own file with the smallest possible public surface.
63+
- [ ] Eliminate duplicated low-level helpers such as line/byte indexing by consolidating them in one shared helper module.
64+
- [ ] Run `cargo test -p grapha-swift` after each extraction to keep the split behaviorally neutral.
65+
- [ ] Commit the module split once tests pass without changing CLI output.
66+
67+
### Task 3: Extract CLI orchestration out of `main.rs`
68+
69+
**Files:**
70+
- Modify: `grapha/src/main.rs`
71+
- Create: `grapha/src/app/mod.rs` or `grapha/src/commands/mod.rs`
72+
- Create: `grapha/src/app/pipeline.rs`
73+
- Create: `grapha/src/app/index.rs`
74+
- Create: `grapha/src/app/query.rs`
75+
- Create: `grapha/src/app/serve.rs`
76+
- Create: `grapha/src/cli.rs` if clap types are split from runtime behavior
77+
- Test: `grapha/tests/integration.rs`
78+
79+
- [ ] Preserve the existing command-line contract with a smoke test matrix for `analyze`, `index`, `symbol`, `flow`, `l10n`, `asset`, `repo`, and `serve --mcp`.
80+
- [ ] Move `run_pipeline` and indexing orchestration out of `main.rs` into a dedicated runtime module.
81+
- [ ] Move command-specific handlers (`handle_symbol_command`, `handle_flow_command`, `handle_l10n_command`, `handle_asset_command`, `handle_repo_command`) into one or more command modules grouped by behavior rather than by output format.
82+
- [ ] Reduce `main()` to parsing CLI args, building render options, and dispatching into extracted runtime functions.
83+
- [ ] Keep watch-mode logic out of the CLI parsing layer by isolating it behind a `serve`/`watch` runtime module.
84+
- [ ] Run `cargo test -p grapha -- integration` and a manual `cargo run -p grapha -- --help` sanity check.
85+
- [ ] Commit this as a pure structure change with no query semantics changes.
86+
87+
### Task 4: Split SQLite persistence into schema, write, and read paths
88+
89+
**Files:**
90+
- Modify: `grapha/src/store/sqlite.rs`
91+
- Create: `grapha/src/store/sqlite/mod.rs`
92+
- Create: `grapha/src/store/sqlite/schema.rs`
93+
- Create: `grapha/src/store/sqlite/write.rs`
94+
- Create: `grapha/src/store/sqlite/read.rs`
95+
- Create: `grapha/src/store/sqlite/compat.rs`
96+
- Test: move or keep existing `#[cfg(test)]` coverage adjacent to new modules
97+
98+
- [ ] Lock in behavior with focused tests for full save/load, incremental sync, `load_filtered`, and legacy schema compatibility.
99+
- [ ] Extract schema/version constants and table creation helpers into `schema.rs`.
100+
- [ ] Extract full/incremental write paths into `write.rs`, keeping `SqliteStore` as the stable facade type.
101+
- [ ] Extract all load/decode logic into `read.rs` and isolate schema-version-specific edge decoding into `compat.rs`.
102+
- [ ] Replace string-built filtering where possible with parameterized SQL or tightly-scoped helper builders so SQL assembly is explicit and testable.
103+
- [ ] Keep `load_filtered` semantics stable for the localization fast path used in [main.rs:825](/Users/wendell/Developer/oops-rs/grapha/grapha/src/main.rs#L825).
104+
- [ ] Run `cargo test -p grapha sqlite_store` and then full `cargo test`.
105+
- [ ] Commit after the persistence tests and integration tests both pass.
106+
107+
### Task 5: Re-run smell analysis and tighten local thresholds
108+
109+
**Files:**
110+
- Modify: `grapha/src/query/smells.rs` only if threshold tuning or exclusions are needed
111+
- Modify: relevant tests if smell output changes
112+
- Use: generated `.grapha/` index for local verification
113+
114+
- [ ] Re-index the repo with `cargo run -p grapha -- index .`.
115+
- [ ] Run `cargo run -p grapha -- repo smells -p .` and compare the before/after warning set.
116+
- [ ] Confirm the original hotspots are reduced: dead config path removed, `treesitter.rs` split, `main.rs` slimmed down, `sqlite.rs` split.
117+
- [ ] Only adjust smell thresholds if the remaining warnings are demonstrably false positives after the refactors.
118+
- [ ] Capture the remaining warnings in the final PR description so future cleanup has an explicit baseline.
119+
120+
### Task 6: Full verification and integration checkpoint
121+
122+
**Files:**
123+
- No new code expected unless verification exposes regressions
124+
125+
- [ ] Run `cargo fmt -- --check`.
126+
- [ ] Run `cargo clippy --all-targets --all-features`.
127+
- [ ] Run `cargo test`.
128+
- [ ] Run one real-world CLI smoke pass: `cargo run -p grapha -- index .` followed by `cargo run -p grapha -- repo smells -p .`.
129+
- [ ] If all checks pass, prepare either a cleanup PR or continue with any remaining smell-specific follow-up in a new plan rather than extending this refactor indefinitely.
130+
131+
---
132+
133+
## Notes
134+
135+
- The highest-priority fix is Task 1 because it is a correctness issue, not just style.
136+
- Tasks 2 through 4 should be reviewed as structural refactors. Avoid mixing new feature behavior into those commits.
137+
- If the tree-sitter or SQLite splits reveal hidden coupling that would require semantic rewrites, stop and create a follow-up plan rather than forcing the decomposition in one pass.

grapha-core/src/pipeline.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ pub fn file_context(context: &ProjectContext, modules: &ModuleMap, file: &Path)
6969
relative_path,
7070
absolute_path,
7171
module_name,
72+
index_store_enabled: context.index_store_enabled,
7273
}
7374
}
7475

@@ -208,6 +209,7 @@ mod tests {
208209
let project = ProjectContext {
209210
input_path: dir.path().to_path_buf(),
210211
project_root: dir.path().to_path_buf(),
212+
index_store_enabled: true,
211213
};
212214
let mut modules = ModuleMap::new();
213215
modules.modules.insert("core".to_string(), vec![src_dir]);

grapha-core/src/plugin.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ use crate::module::ModuleMap;
1313
pub struct ProjectContext {
1414
pub input_path: PathBuf,
1515
pub project_root: PathBuf,
16+
pub index_store_enabled: bool,
1617
}
1718

1819
impl ProjectContext {
@@ -21,6 +22,7 @@ impl ProjectContext {
2122
input_path: input_path.to_path_buf(),
2223
project_root: std::fs::canonicalize(input_path)
2324
.unwrap_or_else(|_| input_path.to_path_buf()),
25+
index_store_enabled: true,
2426
}
2527
}
2628

@@ -36,6 +38,7 @@ pub struct FileContext {
3638
pub relative_path: PathBuf,
3739
pub absolute_path: PathBuf,
3840
pub module_name: Option<String>,
41+
pub index_store_enabled: bool,
3942
}
4043

4144
pub trait GraphPass: Send + Sync {

grapha-swift/src/lib.rs

Lines changed: 96 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,11 @@ impl LanguagePlugin for SwiftPlugin {
4747
}
4848

4949
fn prepare_project(&self, context: &ProjectContext) -> anyhow::Result<()> {
50-
prepare_project_index_store(&context.project_root);
50+
if context.index_store_enabled {
51+
prepare_project_index_store(&context.project_root);
52+
} else {
53+
clear_index_store_path(&context.project_root);
54+
}
5155
Ok(())
5256
}
5357

@@ -63,6 +67,7 @@ impl LanguagePlugin for SwiftPlugin {
6367
&context.relative_path,
6468
None,
6569
Some(&context.project_root),
70+
context.index_store_enabled,
6671
)
6772
}
6873

@@ -172,6 +177,17 @@ fn prepare_project_with<F>(
172177
let _ = refresh_index_store_with(cache, project_root, discover);
173178
}
174179

180+
fn set_index_store_path_in(
181+
cache: &RwLock<HashMap<PathBuf, Option<PathBuf>>>,
182+
project_root: &Path,
183+
store: Option<PathBuf>,
184+
) {
185+
cache
186+
.write()
187+
.expect("index-store cache poisoned")
188+
.insert(project_cache_key(project_root), store);
189+
}
190+
175191
/// Force index-store rediscovery for a project, including after a cached miss.
176192
pub fn refresh_index_store(project_root: &Path) -> Option<PathBuf> {
177193
refresh_index_store_with(&INDEX_STORE_PATHS, project_root, discover_index_store)
@@ -228,6 +244,19 @@ pub fn index_store_path(project_root: &Path) -> Option<PathBuf> {
228244
index_store_path_in(&INDEX_STORE_PATHS, project_root)
229245
}
230246

247+
/// Seed or clear the cached index-store path for a project.
248+
///
249+
/// This is primarily useful for tests and for explicit cache invalidation when
250+
/// a caller knows index-store should not be used for a project.
251+
pub fn set_index_store_path(project_root: &Path, store: Option<PathBuf>) {
252+
set_index_store_path_in(&INDEX_STORE_PATHS, project_root, store);
253+
}
254+
255+
/// Clear any cached index-store path for a project.
256+
pub fn clear_index_store_path(project_root: &Path) {
257+
set_index_store_path(project_root, None);
258+
}
259+
231260
fn index_store_path_in(
232261
cache: &RwLock<HashMap<PathBuf, Option<PathBuf>>>,
233262
project_root: &Path,
@@ -366,13 +395,40 @@ pub fn extract_swift(
366395
file_path: &Path,
367396
explicit_index_store_path: Option<&Path>,
368397
project_root: Option<&Path>,
398+
index_store_enabled: bool,
369399
) -> anyhow::Result<ExtractionResult> {
370-
if let Some(root) = project_root {
371-
init_index_store(root);
400+
extract_swift_with_init(
401+
source,
402+
file_path,
403+
explicit_index_store_path,
404+
project_root,
405+
index_store_enabled,
406+
init_index_store,
407+
)
408+
}
409+
410+
fn extract_swift_with_init<F>(
411+
source: &[u8],
412+
file_path: &Path,
413+
explicit_index_store_path: Option<&Path>,
414+
project_root: Option<&Path>,
415+
index_store_enabled: bool,
416+
mut init_index_store_fn: F,
417+
) -> anyhow::Result<ExtractionResult>
418+
where
419+
F: FnMut(&Path),
420+
{
421+
if index_store_enabled && let Some(root) = project_root {
422+
init_index_store_fn(root);
372423
}
373-
let effective_store = explicit_index_store_path
374-
.map(Path::to_path_buf)
375-
.or_else(|| project_root.and_then(index_store_path));
424+
425+
let effective_store = if index_store_enabled {
426+
explicit_index_store_path
427+
.map(Path::to_path_buf)
428+
.or_else(|| project_root.and_then(index_store_path))
429+
} else {
430+
None
431+
};
376432

377433
if let Some(store_path) = effective_store.as_deref() {
378434
// Index store needs absolute file path for matching
@@ -757,6 +813,40 @@ mod discovery_cache_tests {
757813
}
758814
}
759815

816+
#[cfg(test)]
817+
mod index_store_toggle_tests {
818+
use super::extract_swift_with_init;
819+
use std::cell::Cell;
820+
use std::path::Path;
821+
822+
#[test]
823+
fn disabled_index_store_skips_initializer() {
824+
let initializer_called = Cell::new(false);
825+
let source = br#"
826+
import SwiftUI
827+
828+
struct ContentView: View {
829+
var body: some View {
830+
Text("Hello")
831+
}
832+
}
833+
"#;
834+
835+
let result = extract_swift_with_init(
836+
source,
837+
Path::new("ContentView.swift"),
838+
None,
839+
None,
840+
false,
841+
|_| initializer_called.set(true),
842+
)
843+
.unwrap();
844+
845+
assert!(!initializer_called.get());
846+
assert!(result.nodes.iter().any(|node| node.name == "ContentView"));
847+
}
848+
}
849+
760850
#[cfg(test)]
761851
mod marker_tests {
762852
use super::{

grapha-swift/src/swiftsyntax.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,8 @@ mod tests {
165165
}
166166
"#;
167167

168-
let result = extract_swift(source, Path::new("ContentView.swift"), None, None).unwrap();
168+
let result =
169+
extract_swift(source, Path::new("ContentView.swift"), None, None, true).unwrap();
169170

170171
let body = result
171172
.nodes

0 commit comments

Comments
 (0)