refactor(core): Remove unused wrapper. Switch from yaml format to json#85
Conversation
|
Warning Review limit reached
More reviews will be available in 53 minutes and 35 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR migrates the entire storage and serialization layer from YAML ( ChangesYAML → JSON Storage Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/frilvault-core/src/workspace/repository/workspace_index_repository.rs (1)
23-33: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPrevent silent index reset when migrating existing workspaces.
Line 27 returns a default index when
workspace.jsonis absent, and Line 58 persists that default. For users upgrading from YAML-only index files, this silently drops prior index state instead of migrating/failing explicitly.Also applies to: 51-59
🤖 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_index_repository.rs` around lines 23 - 33, When the load method detects that the workspace index file does not exist at line 27, instead of returning a default WorkspaceIndex, check for legacy YAML index files in the workspace directory. Implement migration logic to convert the existing YAML format to the new JSON format, or fail explicitly with a clear error message indicating the migration issue, rather than silently returning a default index that would drop prior workspace state.
🤖 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/constants.rs`:
- Line 2: The change to NOTE_FILE_EXTENSION breaks backward compatibility with
existing vaults using YAML files. Keep both the JSON extension constant and add
a legacy YAML extension constant in the constants.rs file, then update the file
loading logic throughout the codebase to first attempt reading from the JSON
file path, fall back to reading from the YAML file path if JSON is not found,
and automatically rewrite the loaded data back to JSON format to migrate the
vault forward. This ensures existing YAML vaults remain discoverable and
functional while new data is stored in JSON.
In `@crates/frilvault-core/src/storage/repository.rs`:
- Around line 56-58: The current implementation only writes to JSON format
without handling the migration of existing legacy .yml note files, causing them
to become inaccessible after upgrade. Before the write operation in the
serialize and fs::write calls, implement a migration or fallback mechanism that
detects existing .yml files and converts them to the new JSON format.
Alternatively, ensure the read path checks for and loads from existing .yml
files if the corresponding .json file doesn't exist, preserving backward
compatibility during the transition period before fully switching to JSON-only
storage.
---
Outside diff comments:
In
`@crates/frilvault-core/src/workspace/repository/workspace_index_repository.rs`:
- Around line 23-33: When the load method detects that the workspace index file
does not exist at line 27, instead of returning a default WorkspaceIndex, check
for legacy YAML index files in the workspace directory. Implement migration
logic to convert the existing YAML format to the new JSON format, or fail
explicitly with a clear error message indicating the migration issue, rather
than silently returning a default index that would drop prior workspace state.
🪄 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: 7eca1424-b209-4c6e-a0f6-744eb7e126f2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
README.mdcrates/frilvault-core/Cargo.tomlcrates/frilvault-core/src/app/frilvault.rscrates/frilvault-core/src/constants.rscrates/frilvault-core/src/error/errors.rscrates/frilvault-core/src/note/entity/anchor.rscrates/frilvault-core/src/note/entity/line_anchor.rscrates/frilvault-core/src/note/entity/mod.rscrates/frilvault-core/src/note/entity/note_anchor.rscrates/frilvault-core/src/note/entity/symbol_anchor.rscrates/frilvault-core/src/note/entity/symbol_kind.rscrates/frilvault-core/src/parser/json_parser.rscrates/frilvault-core/src/parser/mod.rscrates/frilvault-core/src/parser/note_parser.rscrates/frilvault-core/src/parser/yaml_parser.rscrates/frilvault-core/src/runtime/vault_context.rscrates/frilvault-core/src/storage/mod.rscrates/frilvault-core/src/storage/repository.rscrates/frilvault-core/src/tests/helper.rscrates/frilvault-core/src/tests/mod.rscrates/frilvault-core/src/tests/note_resolver_test.rscrates/frilvault-core/src/tests/note_service_test.rscrates/frilvault-core/src/tests/parser_test.rscrates/frilvault-core/src/tests/repository_test.rscrates/frilvault-core/src/tests/yaml_parser_test.rscrates/frilvault-core/src/workspace/path/path_resolver.rscrates/frilvault-core/src/workspace/repository/workspace_index_repository.rscrates/frilvault-core/src/workspace/repository/workspace_repository.rsdocs/ARCHITECTURE.mddocs/CURRENT_STATE.mddocs/ROADMAP.md
💤 Files with no reviewable changes (7)
- crates/frilvault-core/src/tests/yaml_parser_test.rs
- crates/frilvault-core/src/note/entity/line_anchor.rs
- crates/frilvault-core/src/note/entity/symbol_kind.rs
- crates/frilvault-core/src/parser/yaml_parser.rs
- crates/frilvault-core/src/parser/note_parser.rs
- crates/frilvault-core/src/note/entity/symbol_anchor.rs
- crates/frilvault-core/src/note/entity/note_anchor.rs
| @@ -1,7 +1,7 @@ | |||
| pub const VAULT_DIR_NAME: &str = ".vault"; | |||
| pub const NOTE_FILE_EXTENSION: &str = "yml"; | |||
| pub const NOTE_FILE_EXTENSION: &str = "json"; | |||
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Add a backward-compatible read path for legacy YAML vault data.
Changing both constants to JSON-only makes existing *.yml notes and workspace.yml undiscoverable unless migration/fallback is guaranteed. That is a persisted storage contract break for existing vaults.
Minimal compatibility bridge
pub const NOTE_FILE_EXTENSION: &str = "json";
+pub const LEGACY_NOTE_FILE_EXTENSION: &str = "yml";
...
pub const WORKSPACE_FILE_NAME: &str = "workspace.json";
+pub const LEGACY_WORKSPACE_FILE_NAME: &str = "workspace.yml";Then load-path logic should: try JSON first, fallback to legacy YAML once, then rewrite JSON.
Also applies to: 7-7
🤖 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/constants.rs` at line 2, The change to
NOTE_FILE_EXTENSION breaks backward compatibility with existing vaults using
YAML files. Keep both the JSON extension constant and add a legacy YAML
extension constant in the constants.rs file, then update the file loading logic
throughout the codebase to first attempt reading from the JSON file path, fall
back to reading from the YAML file path if JSON is not found, and automatically
rewrite the loaded data back to JSON format to migrate the vault forward. This
ensures existing YAML vaults remain discoverable and functional while new data
is stored in JSON.
| let json = self.parser.serialize(note_file)?; | ||
|
|
||
| fs::write(note_path, yaml)?; | ||
| fs::write(note_path, json)?; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Add a legacy-note migration/fallback path before fully switching writes to JSON.
On Line 56 and Line 58, writes are JSON-only. Combined with JSON path resolution/loading, existing .yml note files become unreadable after upgrade (reads resolve missing .json and fall back to empty/default), which effectively hides previously persisted notes.
🤖 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/storage/repository.rs` around lines 56 - 58, The
current implementation only writes to JSON format without handling the migration
of existing legacy .yml note files, causing them to become inaccessible after
upgrade. Before the write operation in the serialize and fs::write calls,
implement a migration or fallback mechanism that detects existing .yml files and
converts them to the new JSON format. Alternatively, ensure the read path checks
for and loads from existing .yml files if the corresponding .json file doesn't
exist, preserving backward compatibility during the transition period before
fully switching to JSON-only storage.
1 participant
Summary
Remove unused wrapper. Switch from yaml format to json format
Type of Change
Required
cargo checkpassescargo fmt --all --checkpassescargo clippy --workspace --all-targets -- -D warningspassescargo testpassesChecklist
Summary by CodeRabbit
Release Notes
Refactor
Documentation