Add CRUD functionality that works with the CLI#40
Merged
Conversation
📝 WalkthroughWalkthroughThis PR completes CRUD operations on the core NoteService (delete and update), builds a new CLI crate with argument parsing and command handlers, and connects the CLI to the service layer. The workspace configuration is corrected to reference the properly-spelled frilvault-cli package. ChangesCLI Commands and Core CRUD Operations
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)
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: 2
🧹 Nitpick comments (1)
crates/frilvault-core/src/lib.rs (1)
9-9: 💤 Low valueConsider keeping explicit re-exports instead of a glob.
pub use note::*widens the public API surface and will silently re-export any future item added tonote, making the crate's public contract harder to reason about. Named re-exports keep the surface intentional.🤖 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/lib.rs` at line 9, Replace the glob re-export "pub use note::*" with explicit named re-exports from the note module to avoid widening the public API surface; locate the glob in lib.rs and list each public type/function/const from the note module (e.g., pub use note::{...}) so future additions to note won't be exported implicitly—update the export statement to enumerate the current public items of note and add/remove names as the module evolves.
🤖 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 @.vault/src/main.rs.yml:
- Around line 1-10: The .vault/src/main.rs.yml note file is currently tracked
which may be accidental; decide whether it should be private or a checked-in
fixture and act accordingly: if it should be private, add a rule to .gitignore
to exclude `.vault/src/` or `.vault/src/main.rs.yml` (or re-enable the existing
notes ignore), or move the file into an explicit examples/ or fixtures/
directory; if it is an intentional artifact (e.g., test fixture or anchor note
like id 4c019c16-fbed-4374-99a4-7b508c1f801e), commit a short README or comment
near `.vault/src/main.rs.yml` explaining its purpose and update process to
prevent accidental edits and churn.
In `@crates/frilvault-core/src/note/service.rs`:
- Around line 56-75: update_note currently silently succeeds when no note
matches note_id; change it so that after loading notes (in update_note) you
search for the note with notes.iter_mut().find(|note| note.id == note_id) and if
not found return a FrilVaultError::NoteNotFound (instead of proceeding to save).
Move the existing TODO out of the found-branch into the else-path that returns
the error, and only call self.save_notes(...) when the note was actually
updated; apply the same not-found error behavior in delete_note so both
functions consistently return FrilVaultError::NoteNotFound when the target id is
missing.
---
Nitpick comments:
In `@crates/frilvault-core/src/lib.rs`:
- Line 9: Replace the glob re-export "pub use note::*" with explicit named
re-exports from the note module to avoid widening the public API surface; locate
the glob in lib.rs and list each public type/function/const from the note module
(e.g., pub use note::{...}) so future additions to note won't be exported
implicitly—update the export statement to enumerate the current public items of
note and add/remove names as the module evolves.
🪄 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: d2fbd69f-9313-4ac9-a3ff-36e8f62dd6f8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
.vault/src/main.rs.ymlCargo.tomlapps/frilvault-cli/Cargo.tomlapps/frilvault-cli/src/cli/add.rsapps/frilvault-cli/src/cli/delete.rsapps/frilvault-cli/src/cli/list.rsapps/frilvault-cli/src/cli/mod.rsapps/frilvault-cli/src/cli/update.rsapps/frilvault-cli/src/command/add.rsapps/frilvault-cli/src/command/delete.rsapps/frilvault-cli/src/command/list.rsapps/frilvault-cli/src/command/mod.rsapps/frilvault-cli/src/command/update.rsapps/frilvault-cli/src/main.rsapps/frivault-cli/Cargo.tomlapps/frivault-cli/src/main.rscrates/frilvault-core/Cargo.tomlcrates/frilvault-core/src/lib.rscrates/frilvault-core/src/note/service.rscrates/frilvault-core/src/storage/yaml_repository.rscrates/frilvault-core/src/tests/note_service_test.rs
💤 Files with no reviewable changes (2)
- apps/frivault-cli/src/main.rs
- apps/frivault-cli/Cargo.toml
Comment on lines
+1
to
+10
| notes: | ||
| - id: "4c019c16-fbed-4374-99a4-7b508c1f801e" | ||
| source_file: src/main.rs | ||
| anchor: | ||
| type: Line | ||
| line: 10 | ||
| column: 5 | ||
| content: new content | ||
| created_at: "2026-06-03T15:06:39.025847Z" | ||
| updated_at: "2026-06-03T15:09:37.992766Z" No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if .vault/ is in .gitignore
if [ -f .gitignore ]; then
echo "=== Checking .gitignore for .vault/ ==="
rg -n '\.vault' .gitignore || echo "No .vault/ entry found in .gitignore"
else
echo ".gitignore file not found"
fi
echo ""
echo "=== Other .vault files in the repository ==="
fd -H -t f . .vault/Repository: FrilLab/frilvault
Length of output: 220
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== .gitignore relevant lines ==="
if [ -f .gitignore ]; then
rg -n 'vault|\.vault' .gitignore || true
echo ""
echo "=== Full .gitignore (bounded) ==="
# Print only around the vault lines to keep output small
rg -n 'vault|\.vault' .gitignore | cut -d: -f1 | head -n 20 | while read -r ln; do
start=$((ln-3)); end=$((ln+3))
sed -n "${start},${end}p" .gitignore
echo "----"
done
else
echo ".gitignore not found"
fi
echo ""
echo "=== Check whether .vault/src/ is ignored by git ==="
git ls-files --others -i --exclude-standard -- .vault/src/ || true
echo ""
echo "=== Show what exists under .vault/ ==="
fd -H -t f .vault || trueRepository: FrilLab/frilvault
Length of output: 529
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if .vault/src/main.rs.yml is tracked ==="
git ls-files -- .vault/src/main.rs.yml || true
echo ""
echo "=== Check ignore status for .vault/src/main.rs.yml (git check-ignore) ==="
git check-ignore -v -- .vault/src/main.rs.yml || echo "git reports: not ignored"
echo ""
echo "=== Check ignore status for the directory .vault/src/ ==="
git check-ignore -v -- .vault/src/ || echo "git reports: .vault/src/ not ignored"
echo ""
echo "=== List .vault files known to git (tracked only) ==="
git ls-files -- '*.yml' -- .vault || true
echo ""
echo "=== List .vault files on disk (bounded) ==="
# Avoid large output; just show the file we know about if present
if [ -f .vault/src/main.rs.yml ]; then
echo "Found: .vault/src/main.rs.yml"
fiRepository: FrilLab/frilvault
Length of output: 562
Clarify whether committing .vault/src/main.rs.yml is intentional (it is tracked and not ignored)
.gitignoreonly ignores.vault/cacheand.vault/index(notes are commented out), and.vault/src/main.rs.ymlis tracked and not excluded by git.- If
.vault/src/*is meant to be user-specific/private, add the proper ignore rule (or move it toexamples//fixtures/). - If it’s an intentional checked-in artifact (e.g., note/anchor fixture), add a brief README/comment explaining purpose and how updates should be handled to avoid churn/merge conflicts.
🤖 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 @.vault/src/main.rs.yml around lines 1 - 10, The .vault/src/main.rs.yml note
file is currently tracked which may be accidental; decide whether it should be
private or a checked-in fixture and act accordingly: if it should be private,
add a rule to .gitignore to exclude `.vault/src/` or `.vault/src/main.rs.yml`
(or re-enable the existing notes ignore), or move the file into an explicit
examples/ or fixtures/ directory; if it is an intentional artifact (e.g., test
fixture or anchor note like id 4c019c16-fbed-4374-99a4-7b508c1f801e), commit a
short README or comment near `.vault/src/main.rs.yml` explaining its purpose and
update process to prevent accidental edits and churn.
Comment on lines
+56
to
75
| pub fn update_note( | ||
| &self, | ||
| source_file: impl AsRef<Path>, | ||
| note_id: Uuid, | ||
| content: String, | ||
| ) -> FrilVaultResult<()> { | ||
| let source_file = source_file.as_ref(); | ||
|
|
||
| let mut notes = self.load_notes(source_file)?; | ||
|
|
||
| if let Some(note) = notes.iter_mut().find(|note| note.id == note_id) { | ||
| // TODO: FrilVaultError::NoteNotFound | ||
| note.content = content; | ||
| note.updated_at = Utc::now(); | ||
| } | ||
|
|
||
| self.save_notes(source_file, notes)?; | ||
|
|
||
| Ok(note_file.notes) | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
update_note silently succeeds when the note is missing.
When no note matches note_id, the if let Some(...) block is skipped and the method still calls save_notes and returns Ok(()). The downstream CLI handler (apps/frilvault-cli/src/command/update.rs) then prints "Note updated" even though nothing changed, which misleads the user. The // TODO: FrilVaultError::NoteNotFound is also misplaced: it sits inside the found branch, but the not-found error should be raised in the else path.
🛠️ Suggested handling for the not-found case
- if let Some(note) = notes.iter_mut().find(|note| note.id == note_id) {
- // TODO: FrilVaultError::NoteNotFound
- note.content = content;
- note.updated_at = Utc::now();
- }
-
- self.save_notes(source_file, notes)?;
+ let note = notes
+ .iter_mut()
+ .find(|note| note.id == note_id)
+ .ok_or(/* FrilVaultError::NoteNotFound */)?;
+
+ note.content = content;
+ note.updated_at = Utc::now();
+
+ self.save_notes(source_file, notes)?;Want me to open an issue to track adding a FrilVaultError::NoteNotFound variant and wiring it into both update_note and delete_note?
🤖 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/note/service.rs` around lines 56 - 75, update_note
currently silently succeeds when no note matches note_id; change it so that
after loading notes (in update_note) you search for the note with
notes.iter_mut().find(|note| note.id == note_id) and if not found return a
FrilVaultError::NoteNotFound (instead of proceeding to save). Move the existing
TODO out of the found-branch into the else-path that returns the error, and only
call self.save_notes(...) when the note was actually updated; apply the same
not-found error behavior in delete_note so both functions consistently return
FrilVaultError::NoteNotFound when the target id is missing.
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
Add CRUD functionality that works with the CLI
Closes #11
Closes #13
Closes #14
Closes #15
Closes #16
Closes #17
Closes #18
Closes #19
Type of Change
Required
cargo checkpassescargo fmt --checkpassescargo clippy --workspace --all-targets -- -D warningspassescargo testpassesChecklist
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores