Use standardized hyprnote data dirs and replace bundle IDs #1687
Conversation
Consolidate application data paths to a single "hyprnote" directory and remove app-specific bundle identifiers across the codebase. This change updates multiple modules, scripts, and tests to use dirs::data_dir()/home paths (hyprnote/models, hyprnote/sessions, hyprnote/models/stt, hyprnote/models/llm, Library/Logs/hyprnote, etc.), simplifies path handling (removing app.path().app_data_dir() usage), adds dirs dependency where needed, and adjusts Taskfile and scripts to point to the new centralized path. The change was needed to standardize storage locations, improve compatibility, and avoid scattered bundle-specific directories.
|
Warning Rate limit exceeded@yujonglee has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR updates directory path resolution across the codebase from using bundle identifiers (e.g., Changes
Sequence Diagram(s)sequenceDiagram
participant Plugin as Plugin<br/>(e.g., db/stt)
participant OldPath as Old: Tauri<br/>app_data_dir()
participant NewPath as New: dirs::data_dir<br/>/hyprnote/...
rect rgb(200, 220, 240)
Note over Plugin,NewPath: Path Resolution Migration
Plugin->>OldPath: Get app-specific data path
OldPath-->>Plugin: /Users/.../com.hyprnote.*/...
Plugin->>NewPath: Get system data directory
NewPath-->>Plugin: /Users/.../hyprnote/...
end
rect rgb(240, 220, 200)
Note over Plugin,NewPath: File Access (unchanged logic)
Plugin->>NewPath: Create/read files
NewPath-->>Plugin: File operations succeed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/db2/src/ext.rs (1)
30-38: Replace.unwrap()calls with proper error handling.The function returns
Result<(), crate::Error>but contains two.unwrap()calls that can panic:
- Line 30:
dirs::data_dir().unwrap()- panics if the system data directory cannot be determined (rare but possible on misconfigured systems)- Lines 34-38:
.build().await.unwrap()- panics on database initialization errorsBoth should propagate errors using the
?operator instead.Apply this diff to fix the error handling:
} else { - let dir_path = dirs::data_dir().unwrap().join("hyprnote"); + let dir_path = dirs::data_dir() + .ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, "Could not determine data directory"))? + .join("hyprnote"); std::fs::create_dir_all(&dir_path)?; let file_path = dir_path.join("db.sqlite"); hypr_db_core::DatabaseBuilder::default() .local(file_path) .build() .await - .unwrap() + .map_err(|e| crate::Error::HyprDbError(e))? }plugins/local-llm/src/lib.rs (1)
72-88: Migration logic may be looking in the wrong location.The backward compatibility code appears to have an issue. Line 72 sets
data_dirto the NEW location (dirs::data_dir().unwrap().join("hyprnote")), but then line 79 tries to migrate files FROMdata_dir. This means the migration is looking in the new location instead of the oldapp_data_dir()location where the files actually exist.To fix this, the migration logic should reference the old app data directory:
- let data_dir = dirs::data_dir().unwrap().join("hyprnote"); let models_dir = app.models_dir(); // for backward compatibility { let _ = std::fs::create_dir_all(&models_dir); + // Look in the old app_data_dir location for files to migrate + let old_data_dir = app.path().app_data_dir().unwrap(); - if let Ok(entries) = std::fs::read_dir(&data_dir) { + if let Ok(entries) = std::fs::read_dir(&old_data_dir) { for entry in entries.flatten() { let path = entry.path(); if path.extension().and_then(|ext| ext.to_str()) == Some("gguf") { let new_path = models_dir.join(path.file_name().unwrap()); let _ = std::fs::rename(path, new_path); } } } }
🧹 Nitpick comments (3)
plugins/db/src/ext.rs (1)
38-39: Consider extracting common directory resolution logic.Both
plugins/db/src/ext.rsandplugins/db2/src/ext.rsuse the same pattern for resolving the hyprnote data directory. The relevant code snippets show thatowhisper/owhisper-config/src/lib.rshas adata_dir()helper function. Consider creating a similar shared helper for hyprnote to reduce duplication and ensure consistency.Example in a shared crate:
pub fn hyprnote_data_dir() -> std::io::Result<std::path::PathBuf> { let dir = dirs::data_dir() .ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, "Could not determine data directory"))? .join("hyprnote"); std::fs::create_dir_all(&dir)?; Ok(dir) }Taskfile.yaml (1)
78-78: Hardcoded username reduces portability.The database path contains a hardcoded username (
yujonglee), making this task non-portable for other developers. Consider using an environment variable or$HOMEto make it more flexible.Apply this diff to make it more portable:
- DB: /Users/yujonglee/Library/Application Support/hyprnote/db.sqlite + DB: $HOME/Library/Application Support/hyprnote/db.sqliteplugins/local-llm/src/ext/plugin.rs (1)
51-56: Consider adding migration logic for existing model files.Users with models in the old directory path won't have them automatically migrated to the new
hyprnote/models/llmlocation. This could result in re-downloading large model files.Consider implementing a one-time migration that checks for models in the old location and moves them to the new location, or at minimum, document this breaking change for users.
Would you like me to help generate migration logic, or should this be tracked in a separate issue?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
Taskfile.yaml(1 hunks)crates/gguf/src/lib.rs(1 hunks)crates/llama/src/lib.rs(1 hunks)crates/transcribe-whisper-local/src/lib.rs(1 hunks)owhisper/owhisper-config/src/lib.rs(1 hunks)owhisper/owhisper-server/src/server.rs(1 hunks)plugins/db/Cargo.toml(1 hunks)plugins/db/src/ext.rs(1 hunks)plugins/db2/Cargo.toml(1 hunks)plugins/db2/src/error.rs(1 hunks)plugins/db2/src/ext.rs(1 hunks)plugins/listener/src/actors/controller.rs(1 hunks)plugins/local-llm/src/ext/plugin.rs(1 hunks)plugins/local-llm/src/lib.rs(1 hunks)plugins/local-stt/src/ext.rs(2 hunks)plugins/misc/Cargo.toml(1 hunks)plugins/misc/src/commands.rs(6 hunks)plugins/tracing/src/commands.rs(1 hunks)plugins/tracing/src/ext.rs(1 hunks)plugins/tracing/src/lib.rs(1 hunks)scripts/info.sh(1 hunks)scripts/swap.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
plugins/listener/src/actors/controller.rs (1)
owhisper/owhisper-config/src/lib.rs (1)
data_dir(52-54)
plugins/db/src/ext.rs (1)
owhisper/owhisper-config/src/lib.rs (1)
data_dir(52-54)
plugins/local-llm/src/lib.rs (1)
owhisper/owhisper-config/src/lib.rs (1)
data_dir(52-54)
plugins/local-stt/src/ext.rs (1)
owhisper/owhisper-config/src/lib.rs (1)
data_dir(52-54)
plugins/misc/src/commands.rs (1)
owhisper/owhisper-config/src/lib.rs (1)
data_dir(52-54)
plugins/tracing/src/ext.rs (1)
plugins/tracing/src/commands.rs (2)
logs_dir(7-9)do_log(13-19)
plugins/db2/src/ext.rs (1)
owhisper/owhisper-config/src/lib.rs (1)
data_dir(52-54)
plugins/local-llm/src/ext/plugin.rs (1)
owhisper/owhisper-config/src/lib.rs (1)
data_dir(52-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: fmt
- GitHub Check: ci (macos, macos-14)
🔇 Additional comments (11)
plugins/db/Cargo.toml (1)
28-28: LGTM!The
dirsdependency is correctly added as a workspace dependency and is necessary for the system data directory resolution inplugins/db/src/ext.rs.plugins/db2/Cargo.toml (1)
27-27: LGTM!The
dirsdependency is correctly added as a workspace dependency and is necessary for the system data directory resolution inplugins/db2/src/ext.rs.plugins/misc/Cargo.toml (1)
27-27: LGTM!The
dirsdependency is correctly added as a workspace dependency.plugins/db2/src/error.rs (1)
11-12: LGTM!The
IoErrorvariant is correctly added with the#[from]attribute to enable automatic error conversion fromstd::io::Error, which is needed for the directory creation operations inext.rs.crates/gguf/src/lib.rs (1)
166-167: LGTM! Test path updated consistently.The test path has been updated to use the standardized
hyprnotedirectory structure, aligning with the PR objectives.crates/llama/src/lib.rs (1)
444-445: LGTM! Test path updated consistently.The model path has been updated to use the standardized
hyprnote/models/llmdirectory structure, consistent with similar changes across the codebase.crates/transcribe-whisper-local/src/lib.rs (1)
19-20: LGTM! Test path updated consistently.The test model path has been updated to use the standardized
hyprnote/models/sttdirectory structure, consistent with similar changes across the codebase.owhisper/owhisper-server/src/server.rs (1)
387-387: LGTM! Test asset path updated consistently.The test model path has been updated to use the standardized
hyprnote/models/sttdirectory structure, consistent with similar changes across the codebase.plugins/listener/src/actors/controller.rs (1)
282-282: <!-- [scratchpad_start]
[task verify_session_migration/final]
[observations]
- controller.rs line 282:
dirs::data_dir().unwrap().join("hyprnote").join("sessions")- misc/commands.rs uses identical path in 6+ locations:
dirs::data_dir().unwrap().join("hyprnote").join("sessions")- local-stt/ext.rs:
dirs::data_dir().unwrap().join("hyprnote")- local-llm/lib.rs:
dirs::data_dir().unwrap().join("hyprnote")- db/ext.rs:
dirs::data_dir().unwrap().join("hyprnote")- NO Tauri app-specific directory methods used for session files (app.path(), app_data_dir, etc.)
- RecorderActor does NOT implement any file migration logic
- No old path references found anywhere
[analysis]- Codebase consistently uses system data_dir for all session file operations
- No evidence of a "change" from Tauri app-specific to system directory
- This is the established pattern throughout the codebase, not a new pattern
- Review comment premise is incorrect: there is no path change occurring
[conclusion]- Review comment is based on false premise
- No actual change from one path to another detected
- No missing migration logic (because no actual change exists)
- Reviewer misidentified or misunderstood the code
[done]plugins/tracing/src/commands.rs (1)
8-8: LGTM! Consistent with updated API.The change correctly calls
logs_dir()without arguments, matching the updated signature inext.rs. Error handling is preserved.plugins/tracing/src/lib.rs (1)
43-45: API usage is correct.The change correctly uses the updated
logs_dir()API. Theunwrap()is acceptable during plugin setup, as initialization should fail fast if the logs directory cannot be created.
Fix nightly/stable mixups and duplicate path logic across scripts and Rust code: update scripts/swap.sh to point NIGHTLY_DIR at the hyprnote-nightly directory, adjust scripts/info.sh to read nightly_user_id from the hyprnote-nightly store.json, and consolidate duplicated model-path construction in plugins/local-stt/src/ext.rs to use the existing models_dir() function (joined with "stt" where appropriate). These changes ensure the swap and info scripts reference distinct installations and eliminate duplicated path logic so model location is resolved centrally.
No description provided.