refactor(core): reduce public api surface#83
Merged
Conversation
📝 WalkthroughWalkthroughRefactors ChangesFrilVault core API refactor, CLI update, and test infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/frilvault-core/src/workspace/repository/workspace_repository.rs (1)
19-27: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove commented-out
loadimplementation to keep API intent unambiguous.Keeping a full disabled method body in comments makes it unclear whether this API is intentionally removed or temporarily bypassed. Prefer deleting it and relying on VCS history.
♻️ Proposed cleanup
- // pub fn load(&self) -> FrilVaultResult<WorkspaceMetadata> { - // let path = self.path_resolver.workspace_metadata_path(); - - // let content = fs::read_to_string(path)?; - - // let metadata = serde_yml::from_str(&content)?; - - // Ok(metadata) - // }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/frilvault-core/src/workspace/repository/workspace_repository.rs` around lines 19 - 27, The commented-out load method in the workspace_repository.rs file creates ambiguity about whether the API is intentionally removed or temporarily disabled. Delete the entire commented-out load function block (including all comments) to keep the API intent clear and rely on version control history instead. This improves code clarity by removing dead code that should not be maintained in the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/frilvault-core/src/tests/mod.rs`:
- Around line 33-34: The repair_engin_test module is currently commented out on
lines 33-34, which removes test coverage for cache invalidation behavior.
Uncomment the module by removing the comment markers from both the #[cfg(test)]
attribute and the mod repair_engin_test declaration. If there are specific
unstable tests within the repair_engin_test module causing issues, mark only
those individual tests with #[ignore] instead of disabling the entire module.
In `@crates/frilvault-core/src/tests/repair_engin_test.rs`:
- Around line 8-45: The test function repair_engine_moves_note_files is
completely commented out, which prevents verification that
RepairEngine::apply_moves actually renames note files on the filesystem.
Uncomment the entire test block starting from the #[test] attribute through the
closing brace to re-enable this test. This test is critical because it verifies
the actual filesystem behavior when moving note files, which cannot be detected
by the remaining cache-only tests.
In `@crates/frilvault-core/src/tests/workspace_repository_test.rs`:
- Around line 34-72: The test functions load_returns_saved_workspace_metadata
and create_if_missing_does_not_overwrite_existing_metadata are currently
commented out with a TODO stub. Uncomment both test functions by removing the
comment markers and delete the TODO comment. These tests are important for
validating critical workspace safety contracts around metadata persistence and
ensuring that create_if_missing does not overwrite existing metadata, and should
be restored as active tests rather than left as stubs.
---
Nitpick comments:
In `@crates/frilvault-core/src/workspace/repository/workspace_repository.rs`:
- Around line 19-27: The commented-out load method in the
workspace_repository.rs file creates ambiguity about whether the API is
intentionally removed or temporarily disabled. Delete the entire commented-out
load function block (including all comments) to keep the API intent clear and
rely on version control history instead. This improves code clarity by removing
dead code that should not be maintained in the codebase.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ce2e85c5-0da5-4463-ae1a-99e8b2031331
📒 Files selected for processing (50)
apps/frilvault-cli/src/command/add.rsapps/frilvault-cli/src/command/delete.rsapps/frilvault-cli/src/command/doctor.rsapps/frilvault-cli/src/command/list.rsapps/frilvault-cli/src/command/repair.rsapps/frilvault-cli/src/command/search.rsapps/frilvault-cli/src/command/stats.rsapps/frilvault-cli/src/command/update.rscrates/frilvault-core/src/app/frilvault.rscrates/frilvault-core/src/error/errors.rscrates/frilvault-core/src/error/mod.rscrates/frilvault-core/src/lib.rscrates/frilvault-core/src/note/dto/mod.rscrates/frilvault-core/src/note/entity/note.rscrates/frilvault-core/src/note/mod.rscrates/frilvault-core/src/note/note_repository.rscrates/frilvault-core/src/note/note_service.rscrates/frilvault-core/src/parser/mod.rscrates/frilvault-core/src/runtime/mod.rscrates/frilvault-core/src/runtime/note_cache.rscrates/frilvault-core/src/runtime/vault_context.rscrates/frilvault-core/src/storage/yaml_repository.rscrates/frilvault-core/src/tests/frilvault_app_test.rscrates/frilvault-core/src/tests/helper.rscrates/frilvault-core/src/tests/mod.rscrates/frilvault-core/src/tests/note_entity_test.rscrates/frilvault-core/src/tests/note_resolver_test.rscrates/frilvault-core/src/tests/note_service_test.rscrates/frilvault-core/src/tests/repair_engin_test.rscrates/frilvault-core/src/tests/vault_context_test.rscrates/frilvault-core/src/tests/workspace_index_repository_test.rscrates/frilvault-core/src/tests/workspace_index_test.rscrates/frilvault-core/src/tests/workspace_repository_test.rscrates/frilvault-core/src/tests/yaml_parser_test.rscrates/frilvault-core/src/tests/yaml_repository_test.rscrates/frilvault-core/src/workspace/diff.rscrates/frilvault-core/src/workspace/entity/mod.rscrates/frilvault-core/src/workspace/entity/repair_suggestion.rscrates/frilvault-core/src/workspace/entity/workspace_index.rscrates/frilvault-core/src/workspace/mod.rscrates/frilvault-core/src/workspace/path/mod.rscrates/frilvault-core/src/workspace/repair_engine.rscrates/frilvault-core/src/workspace/repository/mod.rscrates/frilvault-core/src/workspace/repository/workspace_index_repository.rscrates/frilvault-core/src/workspace/repository/workspace_repository.rscrates/frilvault-core/src/workspace/service/mod.rscrates/frilvault-core/src/workspace/service/workspace_service.rscrates/frilvault-core/src/workspace/snapshot/mod.rscrates/frilvault-core/src/workspace/snapshot/snapshot_manager.rscrates/frilvault-core/src/workspace/snapshot/workspace_snapshot.rs
💤 Files with no reviewable changes (3)
- crates/frilvault-core/src/note/mod.rs
- crates/frilvault-core/src/runtime/mod.rs
- crates/frilvault-core/src/note/note_repository.rs
Comment on lines
+33
to
+34
| // #[cfg(test)] | ||
| // mod repair_engin_test; |
There was a problem hiding this comment.
Re-enable the repair engine test module instead of commenting it out.
Line [33]-Line [34] remove the whole repair_engin_test module from the test build, which drops coverage for cache invalidation behavior. If a specific test is unstable, prefer #[ignore] on that test rather than disabling the full module.
Suggested fix
-// #[cfg(test)]
-// mod repair_engin_test;
+#[cfg(test)]
+mod repair_engin_test;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // #[cfg(test)] | |
| // mod repair_engin_test; | |
| #[cfg(test)] | |
| mod repair_engin_test; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/frilvault-core/src/tests/mod.rs` around lines 33 - 34, The
repair_engin_test module is currently commented out on lines 33-34, which
removes test coverage for cache invalidation behavior. Uncomment the module by
removing the comment markers from both the #[cfg(test)] attribute and the mod
repair_engin_test declaration. If there are specific unstable tests within the
repair_engin_test module causing issues, mark only those individual tests with
#[ignore] instead of disabling the entire module.
Comment on lines
+8
to
45
| // #[test] | ||
| // fn repair_engine_moves_note_files() { | ||
| // let workspace = create_test_workspace(); | ||
| // let workspace_root = workspace.root(); | ||
| // let resolver = PathResolver::new(workspace_root); | ||
| // let vault_context = create_test_vault_context(workspace_root); | ||
| // let mut service = NoteService::new(vault_context.clone()); | ||
|
|
||
| let note_repository = YamlNoteRepository::new(resolver.clone()); | ||
| // // 1. create note for original file | ||
| // service | ||
| // .add_note(AddNoteRequest { | ||
| // source_file: "src/main.rs".into(), | ||
| // anchor: NoteAnchor::Line(LineAnchor { line: 1, column: 1 }), | ||
| // content: "test note".to_string(), | ||
| // }) | ||
| // .unwrap(); | ||
|
|
||
| let index_repository = WorkspaceIndexRepository::new(resolver.clone()); | ||
| // // 2. simulate repair move | ||
| // let moves = vec![FileMove { | ||
| // from: "src/main.rs".to_string(), | ||
| // to: "src/main_renamed.rs".to_string(), | ||
| // confidence: 1.0, | ||
| // }]; | ||
|
|
||
| let vault_context = VaultContext::new(note_repository, index_repository); | ||
| // let mut engine = RepairEngine { vault_context }; | ||
|
|
||
| let mut service = NoteService::new(vault_context.clone()); | ||
| // let repaired = engine.apply_moves(moves).unwrap(); | ||
|
|
||
| // 1. create note for original file | ||
| service | ||
| .add_note(AddNoteRequest { | ||
| source_file: "src/main.rs".into(), | ||
| anchor: NoteAnchor::Line(LineAnchor { line: 1, column: 1 }), | ||
| content: "test note".to_string(), | ||
| }) | ||
| .unwrap(); | ||
|
|
||
| // 2. simulate repair move | ||
| let moves = vec![FileMove { | ||
| from: "src/main.rs".to_string(), | ||
| to: "src/main_renamed.rs".to_string(), | ||
| confidence: 1.0, | ||
| }]; | ||
|
|
||
| let mut engine = RepairEngine { vault_context }; | ||
| // assert_eq!(repaired, 1); | ||
|
|
||
| let repaired = engine.apply_moves(moves).unwrap(); | ||
| // let old_path = resolver.note_path_for_source_file("src/main.rs"); | ||
|
|
||
| assert_eq!(repaired, 1); | ||
| // let new_path = resolver.note_path_for_source_file("src/main_renamed.rs"); | ||
|
|
||
| let old_path = resolver.note_path_for_source_file("src/main.rs"); | ||
|
|
||
| let new_path = resolver.note_path_for_source_file("src/main_renamed.rs"); | ||
|
|
||
| assert!(!old_path.exists()); | ||
| assert!(new_path.exists()); | ||
|
|
||
| fs::remove_dir_all(workspace_root).unwrap(); | ||
| } | ||
| // assert!(!old_path.exists()); | ||
| // assert!(new_path.exists()); | ||
| // } | ||
|
|
There was a problem hiding this comment.
Re-enable executable coverage for note-file move behavior.
Line 8 through Line 45 fully comments out the test that verifies RepairEngine::apply_moves actually renames note files. The remaining test only checks cache state, so filesystem move regressions can now pass unnoticed.
Suggested restoration (adapted to current helpers)
-// #[test]
-// fn repair_engine_moves_note_files() {
-// ...
-// }
+#[test]
+fn repair_engine_moves_note_files() {
+ let workspace = create_test_workspace();
+ let workspace_root = workspace.root();
+
+ let mut service = create_test_note_service(workspace_root);
+ service
+ .add_note(AddNoteRequest {
+ source_file: "src/main.rs".into(),
+ anchor: NoteAnchor::Line(LineAnchor { line: 1, column: 1 }),
+ content: "test note".to_string(),
+ })
+ .unwrap();
+
+ let mut vault_context = create_test_vault_context(workspace_root);
+ let old_path = vault_context.resolve_note_path("src/main.rs");
+ let new_path = vault_context.resolve_note_path("src/main_renamed.rs");
+ assert!(old_path.exists());
+
+ let moves = vec![FileMove {
+ from: "src/main.rs".to_string(),
+ to: "src/main_renamed.rs".to_string(),
+ confidence: 1.0,
+ }];
+
+ let mut engine = RepairEngine { vault_context };
+ let repaired = engine.apply_moves(moves).unwrap();
+
+ assert_eq!(repaired, 1);
+ assert!(!old_path.exists());
+ assert!(new_path.exists());
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/frilvault-core/src/tests/repair_engin_test.rs` around lines 8 - 45,
The test function repair_engine_moves_note_files is completely commented out,
which prevents verification that RepairEngine::apply_moves actually renames note
files on the filesystem. Uncomment the entire test block starting from the
#[test] attribute through the closing brace to re-enable this test. This test is
critical because it verifies the actual filesystem behavior when moving note
files, which cannot be detected by the remaining cache-only tests.
Comment on lines
+34
to
+72
| // TODO: use workspace repo's load fn | ||
| // #[test] | ||
| // fn load_returns_saved_workspace_metadata() { | ||
| // let workspace = create_test_workspace(); | ||
| // let workspace_root = workspace.root(); | ||
| // let resolver = PathResolver::new(workspace_root); | ||
|
|
||
| let resolver = PathResolver::new(&workspace_root); | ||
| // let repository = WorkspaceRepository::new(resolver); | ||
|
|
||
| let repository = WorkspaceRepository::new(resolver); | ||
| // let metadata = WorkspaceMetadata::default(); | ||
|
|
||
| let metadata = WorkspaceMetadata::default(); | ||
| // repository.save(&metadata).unwrap(); | ||
|
|
||
| repository.save(&metadata).unwrap(); | ||
| // let loaded = repository.load().unwrap(); | ||
|
|
||
| let loaded = repository.load().unwrap(); | ||
| // assert_eq!(loaded.version, metadata.version,); | ||
| // } | ||
|
|
||
| assert_eq!(loaded.version, metadata.version,); | ||
| // #[test] | ||
| // fn create_if_missing_does_not_overwrite_existing_metadata() { | ||
| // let workspace = create_test_workspace(); | ||
| // let workspace_root = workspace.root(); | ||
| // let resolver = PathResolver::new(workspace_root); | ||
|
|
||
| fs::remove_dir_all(workspace_root).unwrap(); | ||
| } | ||
|
|
||
| #[test] | ||
| fn create_if_missing_does_not_overwrite_existing_metadata() { | ||
| let workspace_root = | ||
| std::env::temp_dir().join(format!("frilvault-test-{}", uuid::Uuid::new_v4(),)); | ||
|
|
||
| fs::create_dir_all(&workspace_root).unwrap(); | ||
|
|
||
| let resolver = PathResolver::new(&workspace_root); | ||
| // let repository = WorkspaceRepository::new(resolver.clone()); | ||
|
|
||
| let repository = WorkspaceRepository::new(resolver.clone()); | ||
|
|
||
| let metadata = WorkspaceMetadata { | ||
| version: 999, | ||
| ..Default::default() | ||
| }; | ||
| // let metadata = WorkspaceMetadata { | ||
| // version: 999, | ||
| // ..Default::default() | ||
| // }; | ||
|
|
||
| repository.save(&metadata).unwrap(); | ||
| // repository.save(&metadata).unwrap(); | ||
|
|
||
| repository.create_if_missing().unwrap(); | ||
| // repository.create_if_missing().unwrap(); | ||
|
|
||
| let loaded = repository.load().unwrap(); | ||
| // let loaded = repository.load().unwrap(); | ||
|
|
||
| assert_eq!(loaded.version, 999,); | ||
|
|
||
| fs::remove_dir_all(workspace_root).unwrap(); | ||
| } | ||
| // assert_eq!(loaded.version, 999,); | ||
| // } |
There was a problem hiding this comment.
Restore metadata persistence/non-overwrite tests instead of leaving TODO stubs.
Line 34 through Line 72 removes executable checks for load() and for create_if_missing() not overwriting existing metadata. That’s an important contract for workspace safety and is currently unguarded.
Minimal reactivation sketch
-use crate::workspace::{PathResolver, WorkspaceRepository};
+use crate::workspace::{PathResolver, WorkspaceMetadata, WorkspaceRepository};
-// TODO: use workspace repo's load fn
-// #[test]
-// fn load_returns_saved_workspace_metadata() {
-// ...
-// }
+#[test]
+fn load_returns_saved_workspace_metadata() {
+ let workspace = create_test_workspace();
+ let workspace_root = workspace.root();
+ let resolver = PathResolver::new(workspace_root);
+ let repository = WorkspaceRepository::new(resolver);
+
+ let metadata = WorkspaceMetadata::default();
+ repository.save(&metadata).unwrap();
+
+ let loaded = repository.load().unwrap();
+ assert_eq!(loaded.version, metadata.version);
+}
-// #[test]
-// fn create_if_missing_does_not_overwrite_existing_metadata() {
-// ...
-// }
+#[test]
+fn create_if_missing_does_not_overwrite_existing_metadata() {
+ let workspace = create_test_workspace();
+ let workspace_root = workspace.root();
+ let resolver = PathResolver::new(workspace_root);
+ let repository = WorkspaceRepository::new(resolver.clone());
+
+ let metadata = WorkspaceMetadata {
+ version: 999,
+ ..Default::default()
+ };
+ repository.save(&metadata).unwrap();
+ repository.create_if_missing().unwrap();
+
+ let loaded = repository.load().unwrap();
+ assert_eq!(loaded.version, 999);
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/frilvault-core/src/tests/workspace_repository_test.rs` around lines 34
- 72, The test functions load_returns_saved_workspace_metadata and
create_if_missing_does_not_overwrite_existing_metadata are currently commented
out with a TODO stub. Uncomment both test functions by removing the comment
markers and delete the TODO comment. These tests are important for validating
critical workspace safety contracts around metadata persistence and ensuring
that create_if_missing does not overwrite existing metadata, and should be
restored as active tests rather than left as stubs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
refactor(core): reduce public api surface
Type of Change
Required
cargo checkpassescargo fmt --all --checkpassescargo clippy --workspace --all-targets -- -D warningspassescargo testpassesChecklist
Summary by CodeRabbit