refactor(integration): integrate soar with modular crates #123
Conversation
|
Warning Rate limit exceeded@QaidVoid has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 20 minutes and 57 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 (7)
WalkthroughWorkspace-wide refactor: crates moved under Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as soar-cli (command)
participant App as AppState
participant MetaMgr as MetadataManager
participant MetaRepo as MetadataRepository
participant CoreRepo as CoreRepository
participant DB as DieselDatabase
participant SQL as SQLite
CLI->>App: AppState::new() / request action
CLI->>App: metadata_manager()
App->>DB: create/open metadata DBs
DB->>SQL: open + migrations
DB-->>App: DieselDatabase
App->>MetaMgr: build MetadataManager (add repo DBs)
CLI->>MetaMgr: query_repo / query_all
MetaMgr->>MetaRepo: execute query (per-repo)
MetaRepo->>DB: with_conn -> run SELECT
DB->>SQL: SELECT packages...
SQL-->>DB: rows
DB-->>MetaRepo: rows -> deserialized
MetaRepo-->>MetaMgr: Vec<Package>
MetaMgr-->>CLI: results
CLI->>CoreRepo: list_filtered / record_installation
CoreRepo->>DB: with_conn -> run INSERT/SELECT
DB->>SQL: write/read core DB
SQL-->>DB: rows/ok
DB-->>CoreRepo: result
CoreRepo-->>CLI: installed package state
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/soar-cli/src/download.rs (1)
194-197: Usecontinueinstead ofbreakto process remaining links.When
packages.is_empty(), usingbreakstops processing all subsequent links. This differs from the behavior in other branches (e.g., lines 117-119, 126-128) that log errors and continue. Usecontinuefor consistent behavior.🔎 Proposed fix
if packages.is_empty() { error!("Invalid download resource '{}'", link); - break; + continue; }crates/soar-cli/src/inspect.rs (1)
189-196: Bug:{inspect_type}not interpolated in string literal.The prompt on line 191 uses
{inspect_type}inside a regular string literal, not a format string. This will display the literal text{inspect_type}to the user instead of "log" or "script".Suggested fix
if content_length > 1_048_576 { let response = interactive_ask( - "The {inspect_type} file is too large. Do you really want to view it (y/N)?", + &format!("The {inspect_type} file is too large. Do you really want to view it (y/N)?"), )?;
🧹 Nitpick comments (37)
crates/soar-db/src/error.rs (1)
59-69: Consider preserving context in theNotFoundconversion.The conversion from
diesel::result::Error::NotFounduses a generic "Record not found" message, which loses context about what entity was being queried. Callers typically have this context and may prefer to constructDbError::NotFounddirectly with a meaningful message rather than relying on?propagation.This is acceptable as-is if callers handle
NotFoundexplicitly, but worth noting for future debugging.crates/soar-utils/src/fs.rs (1)
42-46: Correct use ofmetadata.is_dir()for symlink-safe behavior.Using
metadata.is_dir()(where metadata comes fromsymlink_metadata) ensures that symlinks are always removed withremove_file, even when pointing to directories. This is the correct behavior.Consider enhancing the function's documentation to explicitly state that symlinks (regardless of their target type) are removed as files, not directories.
Optional: Add test coverage for symlink scenarios
Consider adding tests to verify correct symlink handling:
#[test] fn test_safe_remove_symlink_to_file() { let dir = tempdir().unwrap(); let target = dir.path().join("target_file"); fs::write(&target, "content").unwrap(); let link = dir.path().join("link"); os::unix::fs::symlink(&target, &link).unwrap(); safe_remove(&link).unwrap(); assert!(!link.exists()); assert!(target.exists()); // Target should still exist } #[test] fn test_safe_remove_symlink_to_dir() { let dir = tempdir().unwrap(); let target_dir = dir.path().join("target_dir"); fs::create_dir(&target_dir).unwrap(); let link = dir.path().join("link"); os::unix::fs::symlink(&target_dir, &link).unwrap(); safe_remove(&link).unwrap(); assert!(!link.exists()); assert!(target_dir.exists()); // Target should still exist } #[test] fn test_safe_remove_broken_symlink() { let dir = tempdir().unwrap(); let link = dir.path().join("broken_link"); os::unix::fs::symlink("/nonexistent", &link).unwrap(); safe_remove(&link).unwrap(); assert!(!link.exists()); }crates/soar-utils/src/error.rs (2)
149-159: Consider clarifying CreateSymlink error message wording.The message "from '{from}' to '{target}'" reads slightly counter-intuitively since symlinks are typically described as "at path X pointing to Y". Additionally, the help message "target doesn't already exist" refers to the symlink location (
from), not thetargetpath, which could confuse users.Suggested clarification
- #[error("Failed to create symlink from '{from}' to '{target}'")] + #[error("Failed to create symlink at '{from}' pointing to '{target}'")] #[diagnostic( code(soar_utils::fs::create_symlink), - help("Check if you have write permissions and the target doesn't already exist") + help("Check if you have write permissions and the symlink path doesn't already exist") )]
205-209: Consider derivingDebugforIoContext.
IoOperationderivesDebugandClone, butIoContextdoesn't derive any traits. Adding at leastDebugwould help with debugging and logging context when errors occur.Suggested change
/// Context for filesystem operations. +#[derive(Debug)] pub struct IoContext { path: PathBuf, operation: IoOperation, }crates/soar-db/src/migration.rs (1)
10-10: Consider consistent parameter passing for DbType.
DbTypeisCopy, so passing by value vs reference is functionally equivalent. However, there's inconsistency:
apply_migrationstakes&DbType(line 27)migrate_json_to_jsonbtakesDbTypeby value (line 75)For Copy types, Rust idiom typically prefers passing by value. Consider standardizing to either approach across all functions.
Suggested refactor: Pass DbType by value consistently
pub fn apply_migrations( conn: &mut SqliteConnection, - db_type: &DbType, + db_type: DbType, ) -> Result<(), Box<dyn Error + Send + Sync + 'static>> { loop { - match conn.run_pending_migrations(get_migrations(db_type)) { + match conn.run_pending_migrations(get_migrations(&db_type)) { Ok(_) => break, Err(e) if e.to_string().contains("already exists") => { - mark_first_pending(conn, db_type)?; + mark_first_pending(conn, &db_type)?; } Err(e) => return Err(e), } }Also applies to: 27-27, 75-75
crates/soar-db/src/repository/nest.rs (1)
44-49: Consider returning the inserted entity.Since the workspace enables
returning_clauses_for_sqlite_3_35, you could return the insertedNestwith its auto-generated ID, which can be convenient for callers.🔎 Optional: Return inserted entity
- pub fn insert(conn: &mut SqliteConnection, nest: &NewNest) -> QueryResult<usize> { + pub fn insert(conn: &mut SqliteConnection, nest: &NewNest) -> QueryResult<Nest> { diesel::insert_into(nests::table) .values(nest) - .execute(conn) + .returning(Nest::as_returning()) + .get_result(conn) }crates/soar-db/src/connection.rs (2)
58-62: Consider enabling WAL mode for consistency.The
open_without_migrationsmethod skips setting WAL mode, unlikeopen(). If this connection will be used for writes, the inconsistent journal mode could cause issues when multiple connections access the same database.🔎 Proposed fix
pub fn open_without_migrations<P: AsRef<Path>>(path: P) -> Result<Self, ConnectionError> { let path_str = path.as_ref().to_string_lossy(); - let conn = SqliteConnection::establish(&path_str)?; + let mut conn = SqliteConnection::establish(&path_str)?; + + sql_query("PRAGMA journal_mode = WAL;") + .execute(&mut conn) + .map_err(|e| ConnectionError::BadConnection(e.to_string()))?; + Ok(Self { conn }) }
144-158: Consider ensuring the base directory exists before opening databases.
DatabaseManager::newassumesbase_direxists. If the directory doesn't exist, the database file creation will fail with a potentially unclear error. Consider creating the directory or documenting this precondition.🔎 Proposed fix
pub fn new<P: AsRef<Path>>(base_dir: P) -> Result<Self, ConnectionError> { let base = base_dir.as_ref(); + std::fs::create_dir_all(base) + .map_err(|e| ConnectionError::BadConnection(format!("failed to create db directory: {e}")))?; + let core_path = base.join("core.db"); let nests_path = base.join("nests.db");crates/soar-core/Cargo.toml (1)
14-14: Consider using workspace-managed version forchrono.The
chronodependency uses a direct version spec ("0.4") while most other dependencies useworkspace = true. Ifchronois defined in the workspace manifest, consider using workspace management for consistency and to avoid version drift.#!/bin/bash # Check if chrono is defined in the workspace Cargo.toml grep -A2 'chrono' Cargo.toml | head -5crates/soar-core/src/package/update.rs (1)
15-37: Consider transactional consistency for cleanup operations.The current implementation removes directories from disk in a loop before cleaning up the database. If directory removal fails partway through, you could end up with an inconsistent state where some old versions are removed from disk but all old version records remain in the database.
Consider one of these approaches:
- Collect all removal errors and only delete from DB if all disk removals succeed
- Use a defer/finally pattern to ensure DB cleanup happens even on partial disk removal
- Accept the current behavior but document that partial cleanup may occur
🔎 Option 1: Only clean DB if all disk removals succeed
pub fn remove_old_versions(package: &Package, db: &DieselDatabase) -> SoarResult<()> { let Package { pkg_id, pkg_name, repo_name, .. } = package; let old_packages = db.with_conn(|conn| CoreRepository::get_old_package_paths(conn, pkg_id, pkg_name, repo_name))?; + let mut removal_errors = Vec::new(); for (_id, installed_path) in &old_packages { let path = Path::new(installed_path); if path.exists() { - fs::remove_dir_all(path) - .with_context(|| format!("removing old package directory {}", path.display()))?; + if let Err(e) = fs::remove_dir_all(path) + .with_context(|| format!("removing old package directory {}", path.display())) { + removal_errors.push(e); + } } } + + if !removal_errors.is_empty() { + return Err(removal_errors.into_iter().next().unwrap()); + } db.with_conn(|conn| CoreRepository::delete_old_packages(conn, pkg_id, pkg_name, repo_name))?; Ok(()) }crates/soar-cli/src/update.rs (2)
76-91: Consider extracting duplicate version-check logic into a helper function.The logic for finding newer versions and converting the result is duplicated between the
Some(packages)branch (lines 77-91) and theelsebranch (lines 125-139). This could be extracted to reduce duplication.🔎 Proposed refactor
fn find_newer_package( metadata_mgr: &MetadataManager, pkg: &InstalledPackage, ) -> SoarResult<Option<Package>> { let new_pkg = metadata_mgr .query_repo(&pkg.repo_name, |conn| { MetadataRepository::find_newer_version( conn, &pkg.pkg_name, &pkg.pkg_id, &pkg.version, ) })? .flatten() .map(|p| { let mut package: Package = p.into(); package.repo_name = pkg.repo_name.clone(); package }); Ok(new_pkg) }Also applies to: 125-139
328-328: Silently ignoringremove_old_versionserrors may hide cleanup failures.Using
let _ = remove_old_versions(...)discards any errors from the cleanup operation. While failing to remove old versions may not be critical, silently ignoring these errors could lead to disk space accumulation or stale files that users aren't aware of.Consider logging a warning when cleanup fails:
🔎 Proposed improvement
if !keep { - let _ = remove_old_versions(&target.package, &diesel_db); + if let Err(e) = remove_old_versions(&target.package, &diesel_db) { + warn!("Failed to remove old versions for {}: {}", target.package.pkg_name, e); + } }Also applies to: 341-341
crates/soar-cli/src/nest.rs (2)
4-16: Consider handling duplicate nest name insertions gracefully.The
add_nestfunction doesn't check if a nest with the same name already exists before inserting. Depending on the database schema, this could either:
- Cause a unique constraint violation (which would return an error)
- Create duplicate entries
If there's a unique constraint on
name, the current error message "Failed to add nest" may not clearly indicate a duplicate name issue.🔎 Proposed improvement
pub async fn add_nest(name: &str, url: &str) -> SoarResult<()> { let full_name = format!("nest-{name}"); let mut conn = get_nests_db_conn()?; let nest = NewNest { name: &full_name, url, }; NestRepository::insert(conn.conn(), &nest) - .map_err(|e| SoarError::Custom(format!("Failed to add nest: {}", e)))?; + .map_err(|e| { + if e.to_string().contains("UNIQUE constraint") { + SoarError::Custom(format!("Nest `{name}` already exists")) + } else { + SoarError::Custom(format!("Failed to add nest: {}", e)) + } + })?; println!("Added nest: {}", name); Ok(()) }
4-4: Functions markedasyncbut contain no await points.All three functions (
add_nest,remove_nest,list_nests) are marked asasyncbut perform only synchronous database operations. This adds unnecessary overhead from the async state machine.If these need to be async for API consistency with other CLI commands, this is fine. Otherwise, consider removing the
asynckeyword.Also applies to: 18-18, 34-34
crates/soar-core/src/package/remove.rs (1)
26-28: Constructor is markedasyncbut contains no await points.The
newmethod is marked asasyncbut only performs synchronous struct initialization. Consider removing theasynckeyword unless it's required for API consistency.🔎 Proposed fix
-pub async fn new(package: InstalledPackage, db: DieselDatabase) -> Self { +pub fn new(package: InstalledPackage, db: DieselDatabase) -> Self { Self { package, db } }crates/soar-cli/src/remove.rs (1)
76-88: Clarify the skip condition logic.The condition
query.name.is_none() && !installed_pkg.with_pkg_iddetermines whether to skip removal. Consider adding a comment explaining when this scenario occurs and why these packages should be skipped.crates/soar-cli/src/use.rs (1)
145-147: Consider combining the two transactions.The unlink (line 78-80) and link (line 145-147) operations are in separate transactions. If an error occurs between them (e.g., during symlink manipulation), the database could be left in an inconsistent state where packages are unlinked but the selected one isn't linked.
crates/soar-cli/src/download.rs (1)
263-292: Consider extracting retry logic into a helper.The retry loop with backoff for OCI downloads (lines 263-292) is well-implemented but could be extracted into a reusable utility if similar patterns exist elsewhere in the codebase.
crates/soar-db/src/models/metadata.rs (1)
47-129: Consider using Diesel's#[diesel(deserialize_as)]attribute.The manual
Queryableimplementation with 37-element tuple is verbose and error-prone when adding/removing fields. Consider if Diesel'sdeserialize_asattribute could simplify JSON field handling while maintaining the custom deserialization logic.crates/soar-config/src/display.rs (1)
32-35: Avoid unnecessary clone.
self.progress_styleis anOption<ProgressStyle>, and calling.clone()before.unwrap_or_default()creates an unnecessary copy when the value isSome. Useclone_or_default()or pattern match.Suggested fix
pub fn progress_style(&self) -> ProgressStyle { - self.progress_style.clone().unwrap_or_default() + self.progress_style.clone().unwrap_or_default() }Actually, since
ProgressStyleis a simple enum (cheap to clone), this is acceptable. However, a cleaner pattern would be:pub fn progress_style(&self) -> ProgressStyle { - self.progress_style.clone().unwrap_or_default() + self.progress_style.as_ref().cloned().unwrap_or_default() }Or simply keep as-is since the clone cost is negligible for this enum.
crates/soar-cli/src/health.rs (2)
112-139: Simplify:Rc<RefCell<Vec>>is unnecessary here.The closures passed to
walk_dirdon't escape the function scope, so a simple mutable reference would suffice. TheRc<RefCell>adds complexity without benefit.Suggested simplification
fn get_broken_symlinks() -> SoarResult<Vec<std::path::PathBuf>> { - let broken_symlinks = Rc::new(RefCell::new(Vec::new())); - - let broken_symlinks_clone = Rc::clone(&broken_symlinks); - let mut collect_action = |path: &Path| -> FileSystemResult<()> { + let mut broken_symlinks = Vec::new(); + + let mut collect_action = |path: &Path| -> FileSystemResult<()> { if !path.exists() { - broken_symlinks_clone.borrow_mut().push(path.to_path_buf()); + broken_symlinks.push(path.to_path_buf()); } Ok(()) }; - - let mut soar_files_action = |path: &Path| -> FileSystemResult<()> { - if let Some(filename) = path.file_stem().and_then(|s| s.to_str()) { - if filename.ends_with("-soar") && !path.exists() { - broken_symlinks_clone.borrow_mut().push(path.to_path_buf()); - } - } - Ok(()) - }; - walk_dir(&get_config().get_bin_path()?, &mut collect_action)?; - walk_dir(desktop_dir(), &mut soar_files_action)?; - walk_dir(icons_dir(), &mut soar_files_action)?; + + let mut soar_files_action = |path: &Path| -> FileSystemResult<()> { + if let Some(filename) = path.file_stem().and_then(|s| s.to_str()) { + if filename.ends_with("-soar") && !path.exists() { + broken_symlinks.push(path.to_path_buf()); + } + } + Ok(()) + }; + walk_dir(desktop_dir(), &mut soar_files_action)?; + walk_dir(icons_dir(), &mut soar_files_action)?; - Ok(Rc::try_unwrap(broken_symlinks) - .unwrap_or_else(|rc| rc.borrow().clone().into()) - .into_inner()) + Ok(broken_symlinks) }
100-110: Consider makingget_broken_packagessynchronous.This function doesn't perform any async operations -
diesel_db.with_conn()is synchronous. Theasynckeyword is unnecessary overhead.Suggested change
-async fn get_broken_packages() -> SoarResult<Vec<(String, String, String)>> { +fn get_broken_packages() -> SoarResult<Vec<(String, String, String)>> {Note: This would require updating the call site at line 22 to remove
.await.crates/soar-cli/src/list.rs (2)
536-594: Complex fold accumulator reduces readability.The 5-tuple accumulator
(installed_count, unique_count, broken_count, installed_size, broken_size)is hard to follow. Consider using a named struct or breaking this into a more explicit loop.Suggested approach
struct PackageStats { installed_count: u32, unique_count: u32, broken_count: u32, installed_size: u64, broken_size: u64, } // Then use a simple for loop with mutable stats let mut stats = PackageStats::default(); for package in &packages { // ... update stats }
60-66: Consider extracting the installed packages lookup pattern.The same pattern for building a
HashMapof installed packages appears in bothsearch_packages(lines 60-66) andlist_packages(lines 407-413). Consider extracting this to a helper function to reduce duplication.Example helper
fn get_installed_packages_map( diesel_db: &DieselDatabase, ) -> SoarResult<HashMap<(String, String, String), bool>> { diesel_db .with_conn(|conn| { CoreRepository::list_filtered(conn, None, None, None, None, None, None, None, None) })? .into_par_iter() .map(|pkg| ((pkg.repo_name, pkg.pkg_id, pkg.pkg_name), pkg.is_installed)) .collect() }Also applies to: 407-413
crates/soar-core/src/error.rs (1)
152-170: Consider extendingroot_cause()to leverage thesource()chain for more variants.Currently
root_cause()only provides special handling forUreqErrorandConfig. Other variants with#[source]attributes (likeIoError) would benefit from similar treatment to surface the underlying cause.Optional: Generic source chain traversal
pub fn root_cause(&self) -> String { match self { Self::UreqError(e) => { format!( "Root cause: {}", e.source() .map_or_else(|| e.to_string(), |source| source.to_string()) ) } Self::Config(err) => err.to_string(), - _ => self.to_string(), + Self::IoError { source, .. } => format!("Root cause: {}", source), + other => { + // Generic fallback using Error::source() + if let Some(src) = other.source() { + format!("Root cause: {}", src) + } else { + other.to_string() + } + } } }crates/soar-cli/src/state.rs (4)
32-50: Error handling is good, but consider preserving original error type.The function maps database errors to
SoarError::Custom, which loses the structured error information. Consider using a dedicatedDatabaseErrorvariant or propagating theDbErrortype if available.
81-135: Silent failure on individual nest sync may hide issues.Lines 129-130 log errors but continue processing. This is likely intentional for resilience (one failed nest shouldn't block others), but consider whether failed syncs should be collected and reported as a summary warning at the end.
Additionally, line 130's empty
_ => {}arm silently ignoresOk(None)(unchanged/cached). Consider adding a trace log for debugging purposes:Err(err) => error!("Failed to sync nest {}: {err}", nest.name), - _ => {} + Ok(None) => tracing::trace!("[{}] Nest unchanged (cached)", nest.name), }
316-322: Potential performance concern: sync methods called on everymetadata_manager()access.Even with
force=false,init_repo_dbsandsync_nestsare called every timemetadata_manager()is accessed. WhileOnceCell::get_or_try_initensurescreate_metadata_managerruns only once, the sync methods run unconditionally before the check.Consider moving the sync calls inside the initialization closure or caching the sync state:
pub async fn metadata_manager(&self) -> SoarResult<&MetadataManager> { - self.init_repo_dbs(false).await?; - self.sync_nests(false).await?; - self.inner + self.inner .metadata_manager - .get_or_try_init(|| self.create_metadata_manager()) + .get_or_try_init(|| async { + self.init_repo_dbs(false).await?; + self.sync_nests(false).await?; + self.create_metadata_manager() + }.await) }Note: This would require changing
OnceCelltotokio::sync::OnceCellfor async initialization.
245-274: Silent error suppression increate_metadata_managermay hide configuration issues.Lines 249, 257-259 use
if let Ok(...)patterns that silently ignore errors when getting paths or listing nests. While this prevents failures from blocking startup, it could mask misconfigurations. Consider logging warnings when these paths fail.for repo in &self.inner.config.repositories { - if let Ok(repo_path) = repo.get_path() { + match repo.get_path() { + Ok(repo_path) => { let metadata_db = repo_path.join("metadata.db"); if metadata_db.is_file() { manager.add_repo(&repo.name, metadata_db)?; } + } + Err(e) => tracing::warn!("Skipping repo {}: {}", repo.name, e), } }crates/soar-core/src/database/connection.rs (1)
91-107: Consider usingHashMapfor O(1) repository lookups.
MetadataManagerstores databases in aVec<(String, DieselDatabase)>, which results in O(n) lookup time forquery_repoandfind_first. If the number of repositories grows, this could become a performance concern.A
HashMap<String, DieselDatabase>would provide O(1) lookups while still supporting iteration forquery_alloperations.🔎 Proposed refactor
+use std::collections::HashMap; + /// Manager for multiple metadata databases (one per repository). /// Replaces the ATTACH DATABASE pattern with separate connections. pub struct MetadataManager { - databases: Vec<(String, DieselDatabase)>, + databases: HashMap<String, DieselDatabase>, + // Preserve insertion order for deterministic iteration + order: Vec<String>, }crates/soar-cli/src/install.rs (2)
366-373: Unused_stateparameter.The
_stateparameter is prefixed with underscore indicating it's intentionally unused. If it's not needed, consider removing it from the function signature to avoid confusion. If it's planned for future use, a brief comment would help.
160-167: Consider using a type alias for the MetadataManager path.The full path
soar_core::database::connection::MetadataManageris used inline. SinceDieselDatabaseis already imported from this module, consider importingMetadataManageras well for consistency.🔎 Proposed fix
use soar_core::{ - database::{connection::DieselDatabase, models::Package}, + database::{connection::{DieselDatabase, MetadataManager}, models::Package}, error::{ErrorContext, SoarError}, ... }; ... fn resolve_packages( state: &AppState, - metadata_mgr: &soar_core::database::connection::MetadataManager, + metadata_mgr: &MetadataManager, diesel_db: &DieselDatabase,crates/soar-db/src/repository/metadata.rs (1)
134-158: Consider using Diesel'silikefor case-insensitive search if available.The current implementation uses raw SQL for case-insensitive LIKE queries. While functional, this reduces type safety. SQLite's
LIKEis case-insensitive for ASCII by default, so the explicitLOWER()calls may be unnecessary.crates/soar-db/src/repository/core.rs (4)
368-403:#[allow(clippy::too_many_arguments)]suggests potential for a builder pattern.The function has 11 parameters, which is hard to use correctly. Consider introducing a builder or a struct to group related parameters.
🔎 Proposed approach
pub struct InstallationRecord<'a> { pub repo_name: &'a str, pub pkg_name: &'a str, pub pkg_id: &'a str, pub version: &'a str, pub new_version: &'a str, pub size: i64, pub provides: Option<Vec<PackageProvide>>, pub with_pkg_id: bool, pub checksum: Option<&'a str>, pub installed_date: &'a str, } impl CoreRepository { pub fn record_installation( conn: &mut SqliteConnection, record: &InstallationRecord, ) -> QueryResult<Option<i32>> { // ... implementation } }
530-590: Code duplication betweenget_old_package_pathsanddelete_old_packages.Both methods have nearly identical logic for finding the latest package ID and then filtering on the same criteria. Consider extracting the common query builder or reusing
get_old_package_pathsindelete_old_packages.🔎 Proposed refactor
+ /// Helper to find the latest package ID for a given pkg_id, pkg_name, repo_name. + fn find_latest_id( + conn: &mut SqliteConnection, + pkg_id: &str, + pkg_name: &str, + repo_name: &str, + ) -> QueryResult<Option<i32>> { + packages::table + .filter(packages::pkg_id.eq(pkg_id)) + .filter(packages::pkg_name.eq(pkg_name)) + .filter(packages::repo_name.eq(repo_name)) + .order(packages::id.desc()) + .select(packages::id) + .first(conn) + .optional() + } + /// Gets old package versions (all except the newest unpinned one) for cleanup. pub fn get_old_package_paths( conn: &mut SqliteConnection, pkg_id: &str, pkg_name: &str, repo_name: &str, ) -> QueryResult<Vec<(i32, String)>> { - let latest_id: Option<i32> = packages::table - .filter(packages::pkg_id.eq(pkg_id)) - .filter(packages::pkg_name.eq(pkg_name)) - .filter(packages::repo_name.eq(repo_name)) - .order(packages::id.desc()) - .select(packages::id) - .first(conn) - .optional()?; - - let Some(latest_id) = latest_id else { + let Some(latest_id) = Self::find_latest_id(conn, pkg_id, pkg_name, repo_name)? else { return Ok(Vec::new()); }; // ... rest unchanged }
486-522: Upsert pattern is correct but could useon_conflictfor atomicity.The current update-then-insert pattern works but isn't atomic. If two concurrent calls occur, both might see
updated == 0and try to insert, causing a constraint violation.Diesel supports
on_conflictwhich would be more robust, though SQLite'sON CONFLICTbehavior withDO UPDATErequires careful handling.
232-252: Raw SQL forCOUNT(DISTINCT ...)reduces type safety.While this works, the raw SQL string is error-prone. Consider using Diesel's
distinct()method or documenting why raw SQL is necessary.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (84)
Cargo.tomlcrates/soar-cli/Cargo.tomlcrates/soar-cli/build.rscrates/soar-cli/src/cli.rscrates/soar-cli/src/download.rscrates/soar-cli/src/health.rscrates/soar-cli/src/inspect.rscrates/soar-cli/src/install.rscrates/soar-cli/src/list.rscrates/soar-cli/src/logging.rscrates/soar-cli/src/main.rscrates/soar-cli/src/nest.rscrates/soar-cli/src/progress.rscrates/soar-cli/src/remove.rscrates/soar-cli/src/run.rscrates/soar-cli/src/self_actions.rscrates/soar-cli/src/state.rscrates/soar-cli/src/update.rscrates/soar-cli/src/use.rscrates/soar-cli/src/utils.rscrates/soar-config/src/config.rscrates/soar-config/src/display.rscrates/soar-config/src/lib.rscrates/soar-core/CHANGELOG.mdcrates/soar-core/Cargo.tomlcrates/soar-core/src/constants.rscrates/soar-core/src/database/connection.rscrates/soar-core/src/database/mod.rscrates/soar-core/src/database/models.rscrates/soar-core/src/error.rscrates/soar-core/src/lib.rscrates/soar-core/src/package/install.rscrates/soar-core/src/package/mod.rscrates/soar-core/src/package/query.rscrates/soar-core/src/package/remove.rscrates/soar-core/src/package/update.rscrates/soar-core/src/utils.rscrates/soar-db/Cargo.tomlcrates/soar-db/migrations/core/2025-10-19-024753-0000_create_packages/up.sqlcrates/soar-db/migrations/metadata/2025-10-19-095216-0000_create_packages/up.sqlcrates/soar-db/src/connection.rscrates/soar-db/src/error.rscrates/soar-db/src/lib.rscrates/soar-db/src/migration.rscrates/soar-db/src/models/core.rscrates/soar-db/src/models/metadata.rscrates/soar-db/src/models/types.rscrates/soar-db/src/repository/core.rscrates/soar-db/src/repository/metadata.rscrates/soar-db/src/repository/mod.rscrates/soar-db/src/repository/nest.rscrates/soar-db/src/schema/core.rscrates/soar-db/src/schema/metadata.rscrates/soar-registry/src/lib.rscrates/soar-registry/src/metadata.rscrates/soar-registry/src/package.rscrates/soar-utils/Cargo.tomlcrates/soar-utils/src/error.rscrates/soar-utils/src/fs.rssoar-cli/src/health.rssoar-cli/src/list.rssoar-cli/src/nest.rssoar-cli/src/remove.rssoar-cli/src/state.rssoar-cli/src/use.rssoar-core/migrations/core/V5_baseline.sqlsoar-core/migrations/core/V6_add_portable_cache.sqlsoar-core/migrations/metadata/V1_initial.sqlsoar-core/migrations/nests/V1_initial.sqlsoar-core/src/constants.rssoar-core/src/database/connection.rssoar-core/src/database/migration.rssoar-core/src/database/mod.rssoar-core/src/database/models.rssoar-core/src/database/nests/mod.rssoar-core/src/database/nests/models.rssoar-core/src/database/nests/repository.rssoar-core/src/database/packages/mod.rssoar-core/src/database/packages/models.rssoar-core/src/database/packages/query.rssoar-core/src/database/repository.rssoar-core/src/database/statements.rssoar-core/src/error.rssoar-core/src/package/update.rs
💤 Files with no reviewable changes (28)
- crates/soar-db/src/schema/core.rs
- crates/soar-registry/src/package.rs
- soar-core/src/constants.rs
- soar-cli/src/remove.rs
- soar-core/migrations/nests/V1_initial.sql
- crates/soar-db/migrations/core/2025-10-19-024753-0000_create_packages/up.sql
- soar-core/migrations/core/V6_add_portable_cache.sql
- soar-cli/src/nest.rs
- soar-cli/src/health.rs
- soar-core/src/error.rs
- crates/soar-db/src/models/types.rs
- soar-core/src/database/nests/repository.rs
- soar-core/src/database/repository.rs
- soar-core/migrations/core/V5_baseline.sql
- soar-core/src/database/nests/mod.rs
- soar-core/src/database/packages/mod.rs
- soar-core/src/database/packages/query.rs
- soar-core/migrations/metadata/V1_initial.sql
- soar-core/src/database/packages/models.rs
- soar-cli/src/use.rs
- soar-core/src/database/statements.rs
- soar-core/src/database/connection.rs
- soar-core/src/database/nests/models.rs
- soar-core/src/database/migration.rs
- soar-core/src/database/mod.rs
- soar-cli/src/list.rs
- soar-cli/src/state.rs
- soar-core/src/database/models.rs
🧰 Additional context used
🧬 Code graph analysis (21)
crates/soar-db/src/repository/mod.rs (1)
crates/soar-db/src/connection.rs (2)
core(181-183)metadata(188-190)
crates/soar-config/src/lib.rs (1)
crates/soar-config/src/config.rs (1)
display(414-416)
crates/soar-core/src/package/update.rs (1)
crates/soar-db/src/repository/core.rs (2)
get_old_package_paths(532-559)delete_old_packages(562-590)
crates/soar-cli/src/utils.rs (4)
crates/soar-cli/src/state.rs (1)
config(277-279)crates/soar-config/src/config.rs (2)
get_config(123-135)display(414-416)crates/soar-utils/src/fs.rs (1)
is_elf(301-306)crates/soar-utils/src/system.rs (1)
platform(9-16)
crates/soar-cli/src/nest.rs (2)
crates/soar-core/src/utils.rs (1)
get_nests_db_conn(98-104)crates/soar-db/src/repository/nest.rs (3)
insert(45-49)delete_by_name(52-54)list_all(13-15)
crates/soar-db/src/error.rs (2)
crates/soar-core/src/error.rs (1)
from(173-175)crates/soar-utils/src/error.rs (1)
from(282-326)
crates/soar-config/src/display.rs (1)
crates/soar-core/src/database/connection.rs (1)
default(173-175)
crates/soar-cli/src/inspect.rs (5)
crates/soar-cli/src/utils.rs (3)
display_settings(49-51)interactive_ask(61-74)select_package_interactively(105-123)crates/soar-cli/src/progress.rs (1)
create_spinner(51-68)crates/soar-db/src/repository/core.rs (2)
find_exact(173-191)from(54-80)crates/soar-cli/src/state.rs (1)
new(64-74)crates/soar-db/src/repository/metadata.rs (1)
find_filtered(296-330)
crates/soar-cli/src/progress.rs (2)
crates/soar-config/src/config.rs (2)
display(414-416)new(238-256)crates/soar-cli/src/utils.rs (1)
display_settings(49-51)
crates/soar-registry/src/metadata.rs (3)
crates/soar-config/src/config.rs (2)
get_config(123-135)new(238-256)crates/soar-config/src/repository.rs (1)
sync_interval(58-71)crates/soar-registry/src/nest.rs (1)
new(37-43)
crates/soar-cli/src/download.rs (4)
crates/soar-db/src/connection.rs (2)
metadata(188-190)conn(82-84)crates/soar-core/src/package/query.rs (1)
try_from(20-67)crates/soar-core/src/database/models.rs (2)
repo_name(93-95)repo_name(139-141)crates/soar-db/src/repository/metadata.rs (1)
find_filtered(296-330)
crates/soar-core/src/database/models.rs (2)
crates/soar-db/src/repository/core.rs (1)
from(54-80)crates/soar-db/src/models/metadata.rs (1)
new(140-145)
crates/soar-core/src/database/connection.rs (1)
crates/soar-db/src/connection.rs (4)
conn(82-84)open(33-53)new(144-158)open_metadata(70-79)
crates/soar-registry/src/lib.rs (1)
crates/soar-registry/src/metadata.rs (6)
fetch_metadata(329-407)fetch_metadata_with_etag(427-506)fetch_nest_metadata(88-166)fetch_nest_metadata_with_etag(173-252)fetch_public_key(268-284)process_metadata_content(531-577)
crates/soar-core/src/package/remove.rs (1)
crates/soar-db/src/repository/core.rs (2)
delete_portable(525-528)delete(460-462)
crates/soar-db/src/connection.rs (1)
crates/soar-db/src/migration.rs (3)
sql_query(48-49)apply_migrations(25-40)migrate_json_to_jsonb(73-147)
crates/soar-cli/src/health.rs (7)
crates/soar-cli/src/state.rs (2)
config(277-279)new(64-74)crates/soar-config/src/config.rs (3)
get_config(123-135)new(238-256)display(414-416)crates/soar-core/src/package/remove.rs (2)
remove(30-101)new(26-28)crates/soar-utils/src/fs.rs (1)
walk_dir(167-201)crates/soar-utils/src/path.rs (2)
desktop_dir(141-143)icons_dir(146-148)crates/soar-core/src/package/install.rs (1)
new(59-112)crates/soar-db/src/repository/core.rs (1)
list_broken(146-156)
crates/soar-core/src/utils.rs (2)
crates/soar-config/src/config.rs (1)
get_config(123-135)crates/soar-db/src/connection.rs (2)
conn(82-84)open(33-53)
crates/soar-db/src/repository/metadata.rs (2)
crates/soar-db/src/models/metadata.rs (1)
new(140-145)crates/soar-db/src/models/types.rs (1)
from_string(29-55)
crates/soar-cli/src/update.rs (3)
crates/soar-core/src/package/update.rs (1)
remove_old_versions(15-37)crates/soar-db/src/repository/core.rs (3)
find_exact(173-191)list_filtered(93-143)list_updatable(159-170)crates/soar-db/src/repository/metadata.rs (1)
find_newer_version(335-362)
crates/soar-db/src/repository/core.rs (3)
crates/soar-db/src/repository/metadata.rs (5)
diesel(194-196)sql(145-148)sql(211-214)sql(229-229)sql(352-355)crates/soar-core/src/database/models.rs (12)
from(146-172)from(177-203)from(208-264)from(269-273)repo_name(93-95)repo_name(139-141)pkg_name(81-83)pkg_name(127-129)pkg_id(85-87)pkg_id(131-133)version(89-91)version(135-137)crates/soar-db/src/error.rs (2)
from(60-68)from(72-74)
⏰ 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: coverage
- GitHub Check: test
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
crates/soar-cli/src/install.rs (1)
218-228: The unwrap panic risk from the previous review remains unaddressed.Lines 224-225 still unwrap the result from
select_package_interactively, which can returnNoneif the user cancels the selection. Line 227 also unwraps without guaranteeing the vector is non-empty at that point (though theis_empty()check at line 218 provides some protection for that specific path).crates/soar-core/src/package/install.rs (1)
248-264: Critical: The version parameter bug and unsafe default from the previous review remain unfixed.Line 255 still incorrectly passes
versiontwice torecord_installation. Based on the function signature, the second parameter should be the new version, but it's receiving the sameversionvalue again. This prevents the new version from being recorded correctly.Additionally, line 264's
unwrap_or(0)silently converts a failed insert intorecord_id = 0. The subsequentupsert_portablecall at line 298 will then attempt to update records withpackage_id=0, which could corrupt data or silently fail.These are the same issues flagged in the previous review and represent critical correctness bugs.
Proposed fix
let record_id: Option<i32> = self.db.with_conn(|conn| { CoreRepository::record_installation( conn, repo_name, pkg_name, pkg_id, version, - version, + version, // TODO: Should this be a different new_version parameter? size, provides, with_pkg_id, checksum, &installed_date, ) })?; -let record_id = record_id.unwrap_or(0); +let record_id = record_id.ok_or_else(|| { + SoarError::Custom(format!( + "Failed to record installation for {}#{}", + pkg_name, pkg_id + )) +})?;Verify the intended signature of
CoreRepository::record_installationto confirm the correct parameter:#!/bin/bash # Search for the record_installation function definition rg "fn record_installation" -A 15 crates/soar-db/src/repository/
🧹 Nitpick comments (3)
crates/soar-core/Cargo.toml (1)
14-16: Unify dependency management approach for consistency.
chronois pinned to "0.4" whiledieselandmietteuseworkspace = true. For easier maintenance and to match the codebase pattern, consider movingchronoto workspace management.The Diesel workspace configuration is properly set up with the required sqlite backend feature, so no changes needed there.
crates/soar-cli/src/install.rs (1)
253-292: Consider extracting the repeated metadata query pattern.Lines 253-292 duplicate the query logic from lines 177-216 (query single repo vs. all repos, map packages with repo_name). This pattern appears multiple times in the file. Consider extracting a helper function like
query_packages_with_repo_nameto reduce duplication and improve maintainability.Example helper function
fn query_packages_with_repo_name( metadata_mgr: &MetadataManager, repo_name: Option<&str>, pkg_name: Option<&str>, pkg_id: Option<&str>, version: Option<&str>, sort: Option<SortDirection>, ) -> SoarResult<Vec<Package>> { if let Some(ref repo) = repo_name { Ok(metadata_mgr .query_repo(repo, |conn| { MetadataRepository::find_filtered(conn, pkg_name, pkg_id, version, None, sort) })? .unwrap_or_default() .into_iter() .map(|p| { let mut pkg: Package = p.into(); pkg.repo_name = repo.to_string(); pkg }) .collect()) } else { metadata_mgr.query_all_flat(|repo_name, conn| { let pkgs = MetadataRepository::find_filtered(conn, pkg_name, pkg_id, version, None, sort)?; Ok(pkgs .into_iter() .map(|p| { let mut pkg: Package = p.into(); pkg.repo_name = repo_name.to_string(); pkg }) .collect()) }) } }crates/soar-cli/src/update.rs (1)
320-345: Consider logging errors fromremove_old_versions.Lines 330 and 343 silently ignore errors from
remove_old_versionsusinglet _ =. While this prevents cleanup failures from blocking the update flow, it could mask issues like database corruption or filesystem problems. Consider at least logging these errors at the warning level so they're visible for troubleshooting.Suggested improvement
if !keep { - let _ = remove_old_versions(&target.package, &diesel_db); + if let Err(e) = remove_old_versions(&target.package, &diesel_db) { + warn!("Failed to remove old versions: {}", e); + } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/soar-cli/src/install.rscrates/soar-cli/src/update.rscrates/soar-core/Cargo.tomlcrates/soar-core/src/package/install.rs
🧰 Additional context used
🧬 Code graph analysis (1)
crates/soar-cli/src/install.rs (3)
crates/soar-core/src/package/query.rs (1)
try_from(20-67)crates/soar-db/src/repository/metadata.rs (1)
find_filtered(296-330)crates/soar-db/src/repository/core.rs (1)
list_filtered(93-143)
⏰ 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: coverage
- GitHub Check: test
🔇 Additional comments (16)
crates/soar-core/Cargo.toml (3)
22-22: LGTM! Good architectural separation.The addition of
soar-dbas a workspace dependency aligns well with the refactoring goals to introduce a dedicated database abstraction layer with repository patterns.
26-26: LGTM! Improved consistency.Converting
thiserrorto workspace management improves dependency version consistency across the workspace.
14-29: Verify that chrono version resolves to a secure release.The generic version spec
chrono = "0.4"needs to be checked. Versions of chrono before 0.4.20 had a potential segfault vulnerability (RUSTSEC-2020-0159) in Unix-like systems due to dereferencing a dangling pointer. Ensure the Cargo.lock or resolution confirms a patched version is selected. Additionally, verify that workspace dependencies for diesel and miette are current and free from known advisories.crates/soar-cli/src/install.rs (6)
82-92: Progress style setup looks good.The conditional progress style based on icon settings is well-implemented, providing a graceful fallback to plain text when icons are disabled.
130-158: Clean refactoring to AppState-driven flow.The function properly initializes the state and delegates to the resolution layer. The migration from direct DB connections to state-managed resources is well-structured.
234-250: Repository query pattern correctly implemented.The use of
CoreRepository::list_filteredthrough the Diesel connection is correct and follows the new architectural pattern.
368-447: Package selection logic is correct.The function properly handles existing installs, repo-specific queries, and cross-repo queries. The metadata-driven approach is correctly implemented, though it shares the same duplication pattern noted elsewhere in the file.
480-520: Enhanced display output with icons looks great.The per-package installation details with optional icon support provide a much better user experience. The graceful fallback to plain text characters when icons are disabled is well-implemented.
525-567: Excellent summary presentation with conditional rich formatting.The use of
tabledfor icon-enabled mode provides a polished summary table, while maintaining a clean fallback for plain text mode. All edge cases (no installs, failures, mixed results) are properly handled.crates/soar-cli/src/update.rs (4)
24-39: Clean helper function for finding existing packages.The function correctly uses
CoreRepository::find_exactthrough the Diesel connection wrapper. The signature and implementation are straightforward and correct.
54-117: Update resolution logic is sound.The flow correctly identifies installed packages matching the query, checks for newer versions in the metadata repository, and builds update targets. The use of
find_newer_versionis appropriate for the update use case.
119-166: "Update all" logic properly implemented.The use of
CoreRepository::list_updatablecorrectly identifies packages eligible for updates, and the subsequent metadata queries follow the same sound pattern as the selective update path.
230-275: Update summary follows the same polished pattern as install.The summary presentation is consistent with
install.rs, providing clear feedback with optional rich formatting. All result scenarios are properly handled.crates/soar-core/src/package/install.rs (3)
59-113: Constructor properly migrated to repository-based insertion.The refactoring correctly uses
NewInstalledPackageandCoreRepository::insertinstead of direct SQL. The addition of timestamp tracking with chrono and thepinnedfield support are appropriate enhancements.
275-289: Path resolution logic is well-implemented.The
resolve_pathclosure properly handles empty strings, absolute paths, and relative paths, ensuring consistent path normalization for portable configurations. This is a good defensive programming practice.
314-361: Alternate package cleanup correctly migrated to repository pattern.The use of
CoreRepository::find_alternatesandInstalledPackageWithPortableproperly replaces the previous direct SQL approach while maintaining the same cleanup behavior for symlinks and provides.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/soar-registry/src/metadata.rs (2)
64-93: Fix misleading documentation about ETag source.Line 73 states the
existing_etagis "read from the database," but the function no longer reads it—the caller must provide it. Update the documentation to clarify that the caller is responsible for retrieving and passing the ETag.🔎 Suggested documentation fix
-/// * `existing_etag` - Optional etag from a previous fetch, read from the database +/// * `existing_etag` - Optional etag from a previous fetch to enable conditional requests
202-249: Fix misleading documentation about ETag source.Line 212 states the
existing_etagis "read from the database," but the caller must provide it. This is the same documentation issue as infetch_nest_metadata. Update to clarify caller responsibility.🔎 Suggested documentation fix
-/// * `existing_etag` - Optional etag from a previous fetch, read from the database +/// * `existing_etag` - Optional etag from a previous fetch to enable conditional requestsHowever, the example code (lines 236-237) correctly demonstrates the new signature. Good update there!
♻️ Duplicate comments (3)
crates/soar-db/src/lib.rs (1)
47-53: Silent JSON deserialization errors remain unaddressed.The
json_vec!macro continues to useunwrap_or_default(), which silently swallows deserialization errors. This issue was flagged in previous reviews but has not been resolved. Silent failures can hide data corruption and make debugging difficult.crates/soar-db/src/models/core.rs (1)
28-71: JSON deserialization errors remain silently swallowed.Lines 68-69 use the
json_vec!macro which silently converts deserialization failures to empty vectors viaunwrap_or_default(). This inherits the issue flagged in previous reviews of the macro itself.Additionally, the manual 18-field tuple
Queryableimplementation is fragile—column reordering would break silently. Consider adding a doc comment documenting the expected column order as suggested in previous reviews.crates/soar-db/src/repository/metadata.rs (1)
337-367: Indexing mismatch between Rust and SQL remains unfixed.The issue flagged in previous reviews persists: Rust's
[14..]slice (line 348) and SQL'ssubstr(version, 14)(line 359) extract different substrings due to 0-based vs 1-based indexing.For
"HEAD-20231215-abc123":
- Rust
[14..]extracts"abc123"(starts at 15th character, 1-based)- SQL
substr(version, 14)extracts"-abc123"(starts at 14th character, 1-based)Change SQL to
substr(version, 15)to match Rust's offset.
🧹 Nitpick comments (7)
crates/soar-cli/src/use.rs (1)
85-90: Simplify error handling pattern.The
let _ =assignment followed immediately by?is redundant and confusing. Either remove the assignment or handle the intermediate result if needed.🔎 Proposed fix
- let _ = mangle_package_symlinks( + mangle_package_symlinks( &install_dir, &bin_dir, selected_package.provides.as_deref(), ) - .await?; + .await?;crates/soar-cli/src/state.rs (2)
31-49: Consider verifying file removal success.The function removes the existing metadata file but doesn't verify the removal succeeded before creating a new connection. While
fs::remove_filewill fail if the file doesn't exist (and we handle errors), race conditions could leave the file in an inconsistent state.🔎 Suggested improvement
if metadata_db.exists() { fs::remove_file(metadata_db) .with_context(|| format!("removing metadata file {}", metadata_db.display()))?; } + + // Ensure file is removed before proceeding + if metadata_db.exists() { + return Err(SoarError::Custom(format!( + "metadata file {} still exists after removal", + metadata_db.display() + ))); + }
80-134: LGTM! Consider adding sync metrics.The concurrent nest synchronization with per-nest error handling is well-structured. Errors are logged without blocking other nests from syncing.
Optional enhancement: track and report success/failure counts for better observability:
let mut synced = 0; let mut failed = 0; // ... in loop ... Ok(Some(_)) => { synced += 1; /* ... */ } Err(_) => { failed += 1; /* ... */ } // ... after loop ... info!("Nest sync complete: {} synced, {} failed", synced, failed);crates/soar-cli/src/list.rs (1)
380-403: Consider pagination or warning for large repositories.The function calls
list_all()without any limit, which could return thousands of packages in large repositories. This may cause performance issues or overwhelming output.Consider:
- Adding pagination support
- Implementing a configurable max limit with a warning
- Using streaming/chunked output for large result sets
crates/soar-db/src/repository/core.rs (3)
53-81: Consider extracting portable reference once.The implementation correctly handles the conversion, but lines 74-78 repeatedly call
portable.as_ref(). While this is functionally correct and the compiler likely optimizes it, you could extract the reference once for slightly cleaner code.🔎 Optional refactor to extract portable reference once
impl From<(Package, Option<PortablePackage>)> for InstalledPackageWithPortable { fn from((pkg, portable): (Package, Option<PortablePackage>)) -> Self { + let (portable_path, portable_home, portable_config, portable_share, portable_cache) = + if let Some(ref p) = portable { + ( + p.portable_path.clone(), + p.portable_home.clone(), + p.portable_config.clone(), + p.portable_share.clone(), + p.portable_cache.clone(), + ) + } else { + (None, None, None, None, None) + }; + Self { id: pkg.id, repo_name: pkg.repo_name, pkg_id: pkg.pkg_id, pkg_name: pkg.pkg_name, pkg_type: pkg.pkg_type, version: pkg.version, size: pkg.size, checksum: pkg.checksum, installed_path: pkg.installed_path, installed_date: pkg.installed_date, profile: pkg.profile, pinned: pkg.pinned, is_installed: pkg.is_installed, with_pkg_id: pkg.with_pkg_id, detached: pkg.detached, unlinked: pkg.unlinked, provides: pkg.provides, install_patterns: pkg.install_patterns, - portable_path: portable.as_ref().and_then(|p| p.portable_path.clone()), - portable_home: portable.as_ref().and_then(|p| p.portable_home.clone()), - portable_config: portable.as_ref().and_then(|p| p.portable_config.clone()), - portable_share: portable.as_ref().and_then(|p| p.portable_share.clone()), - portable_cache: portable.as_ref().and_then(|p| p.portable_cache.clone()), + portable_path, + portable_home, + portable_config, + portable_share, + portable_cache, } } }
368-401: Consider using a parameter struct for record_installation.The method has 10 parameters and requires
#[allow(clippy::too_many_arguments)]. Consider introducing a parameter struct (e.g.,InstallationRecord) to improve readability and maintainability.🔎 Example parameter struct approach
pub struct InstallationRecord<'a> { pub repo_name: &'a str, pub pkg_name: &'a str, pub pkg_id: &'a str, pub version: &'a str, pub size: i64, pub provides: Option<Vec<PackageProvide>>, pub with_pkg_id: bool, pub checksum: Option<&'a str>, pub installed_date: &'a str, } pub fn record_installation( conn: &mut SqliteConnection, record: &InstallationRecord, ) -> QueryResult<Option<i32>> { // ... implementation using record.field_name }
590-644: Consider extracting common checksum filter logic.The
unlink_others_by_checksumandlink_by_checksummethods have very similar structure with conditional checksum filtering. While the code is clear, you could reduce duplication by extracting a helper method for building the query filters.🔎 Optional refactor example
fn filter_by_checksum<'a>( query: diesel::query_builder::UpdateStatement< packages::table, diesel::query_builder::WhereClause</* filter type */>, >, pkg_name: &str, pkg_id: &str, checksum: Option<&str>, match_pkg_id: bool, // true to match pkg_id, false to exclude it ) -> /* return type */ { let query = query .filter(packages::pkg_name.eq(pkg_name)) .filter(if match_pkg_id { packages::pkg_id.eq(pkg_id) } else { packages::pkg_id.ne(pkg_id) }); if let Some(cs) = checksum { query.filter(if match_pkg_id { packages::checksum.eq(cs) } else { packages::checksum.ne(cs) }) } else { query } }Note: Diesel's type system can make this refactoring complex. The current duplication is acceptable if extracting the common logic proves too difficult.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/ci.yamlCargo.tomlcrates/soar-cli/src/list.rscrates/soar-cli/src/state.rscrates/soar-cli/src/use.rscrates/soar-core/src/database/connection.rscrates/soar-core/src/package/install.rscrates/soar-db/src/lib.rscrates/soar-db/src/models/core.rscrates/soar-db/src/repository/core.rscrates/soar-db/src/repository/metadata.rscrates/soar-registry/src/lib.rscrates/soar-registry/src/metadata.rs
🧰 Additional context used
🧬 Code graph analysis (4)
crates/soar-core/src/package/install.rs (2)
crates/soar-core/src/database/models.rs (12)
repo_name(93-95)repo_name(139-141)pkg_id(85-87)pkg_id(131-133)pkg_name(81-83)pkg_name(127-129)version(89-91)version(135-137)from(146-172)from(177-203)from(208-264)from(269-273)crates/soar-db/src/repository/core.rs (6)
insert(349-354)record_installation(369-401)from(54-80)upsert_portable(485-520)unlink_others(422-439)find_alternates(301-316)
crates/soar-db/src/models/core.rs (1)
crates/soar-db/src/models/metadata.rs (1)
build(88-128)
crates/soar-db/src/repository/metadata.rs (2)
crates/soar-db/src/models/metadata.rs (1)
new(140-145)crates/soar-db/src/models/types.rs (1)
from_string(29-55)
crates/soar-cli/src/state.rs (5)
crates/soar-core/src/utils.rs (1)
get_nests_db_conn(98-104)crates/soar-db/src/connection.rs (7)
core(181-183)metadata(188-190)conn(82-84)open(33-53)new(144-158)nests(198-200)open_metadata(70-79)crates/soar-registry/src/metadata.rs (3)
fetch_metadata(246-325)fetch_nest_metadata(89-168)write_metadata_db(421-430)crates/soar-db/src/repository/metadata.rs (4)
import_packages(378-399)update_repo_metadata(76-84)exists_by_pkg_id(370-375)get_repo_etag(68-73)crates/soar-core/src/package/install.rs (1)
new(60-113)
⏰ 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: test
- GitHub Check: coverage
🔇 Additional comments (24)
Cargo.toml (2)
3-12: LGTM! Clean workspace restructure.The workspace reorganization to a
crates/subdirectory follows best practices for multi-crate Rust projects, and the addition ofsoar-dbaligns with the Diesel-based migration described in the PR summary.Also applies to: 45-52
64-64: Verify the intentional change from speed to size optimization.The opt-level has been changed from
"3"(optimize for speed) to"z"(optimize for binary size). This trade-off prioritizes smaller binaries at the potential cost of runtime performance.Please confirm this change is intentional and that any performance impact on CLI operations has been evaluated.
crates/soar-registry/src/metadata.rs (2)
112-129: LGTM: ETag handling logic is sound.The use of
unwrap_or_default()provides a clean fallback, and the cache validation logic correctly handles both empty and non-empty ETags in conjunction with theforceflag and sync interval checks.
266-282: LGTM: Consistent ETag handling.The implementation mirrors
fetch_nest_metadataand correctly handles the optional ETag parameter with proper cache validation logic.crates/soar-registry/src/lib.rs (1)
22-23: No action required—the documentation example is correct and all callers have been updated.The
existing_etagparameter was introduced in the initial crate creation with the matching call site already updated incrates/soar-cli/src/state.rs. There are no other callers to update. The function example inlib.rscorrectly demonstrates usage with this parameter.crates/soar-cli/src/use.rs (2)
16-37: LGTM!The early return when no packages are found is appropriate and prevents unnecessary processing.
121-143: Past issue resolved: empty vector check added.The condition now correctly checks
!pkg.is_empty()before callingintegrate_package, preventing the vacuous truth issue flagged in previous reviews.crates/soar-cli/src/state.rs (2)
136-176: LGTM!The repository initialization follows the same solid concurrent pattern as nest syncing, with appropriate error handling and separation of fetch/validate concerns.
178-232: LGTM! Package replacement logic is well-implemented.The validation correctly identifies when installed packages have been replaced in the repository metadata and updates the core DB accordingly. The user-facing logging is informative.
crates/soar-cli/src/list.rs (3)
33-168: LGTM! Search limit is appropriately configurable.The search limit uses a three-tier fallback: user-provided limit → config setting → default of 20. This is flexible and well-designed.
170-373: LGTM!The query function provides comprehensive package details with good formatting. The conditional display of optional fields keeps output clean.
499-678: LGTM!The fold operation efficiently computes installed/broken counts and sizes in a single pass. The display logic clearly distinguishes between healthy and broken installations.
crates/soar-core/src/package/install.rs (3)
59-113: LGTM!The package installation initialization correctly uses the repository pattern with a clean transaction-based insert.
248-268: Past issue resolved: proper error handling for failed insert.The
ok_or_elsepattern (lines 263-268) correctly replaces the previousunwrap_or(0)approach, ensuring the function fails explicitly when package record insertion fails rather than silently proceeding withpackage_id=0.
279-312: LGTM!The path resolution closure elegantly handles empty strings and preserves the distinction between absolute and relative paths before persisting portable metadata.
crates/soar-core/src/database/connection.rs (3)
31-38: Past issue resolved: correct method used for metadata DB.The code now correctly uses
DbConnection::open_metadata(path)instead of the genericopen()method, ensuring only the JSON-to-JSONB migration is applied rather than full migrations.
49-53: Past issue resolved: proper error propagation for mutex lock.The
conn()method now returnsResultand properly propagatesPoisonErrorinstead of panicking on mutex lock failure.
89-179: LGTM!The
MetadataManagerprovides a clean abstraction for querying across multiple repository databases. The various query methods (query_all,query_all_flat,query_repo,find_first) cover common use cases well.crates/soar-db/src/repository/metadata.rs (2)
485-496: Past issue resolved: regex compiled once using OnceLock.The regex is now declared as a static
OnceLock(lines 10-11) and initialized once viaget_or_init()(line 487), eliminating the performance issue of compiling on every call.
406-420: Provides filtering logic is correct. The three conditions—exact name match,recurse_providesflag, and package name followed by a delimiter—correctly handle the intended filtering. Edge cases (e.g.,foobarvsfoo==barvsfoo-bar) are properly handled by the delimiter check.crates/soar-db/src/repository/core.rs (4)
13-51: LGTM! Well-structured data models.The data structures are clear and well-organized. The
InstalledPackageWithPortablestruct effectively represents the joined result of package and portable configuration data.
422-439: Verify unlink_others logic matches intent.The filter on lines 432-434 unlinks packages where
pkg_id != keep_pkg_id OR version != keep_version. This means only packages matching BOTHkeep_pkg_idANDkeep_versionremain linked, which appears correct for keeping a single specific package version linked.Confirm this matches the expected behavior.
463-526: LGTM! Portable configuration methods are well-implemented.The portable package configuration management methods correctly handle CRUD operations, and the
upsert_portablemethod uses the proper update-then-insert pattern for manual upserts.
528-588: LGTM! Cleanup methods correctly identify and remove old versions.Both
get_old_package_pathsanddelete_old_packagescorrectly identify the latest version by ID (descending order) and exclude pinned packages from cleanup. The logic is sound.
| pub fn record_installation( | ||
| conn: &mut SqliteConnection, | ||
| repo_name: &str, | ||
| pkg_name: &str, | ||
| pkg_id: &str, | ||
| version: &str, | ||
| size: i64, | ||
| provides: Option<Vec<PackageProvide>>, | ||
| with_pkg_id: bool, | ||
| checksum: Option<&str>, | ||
| installed_date: &str, | ||
| ) -> QueryResult<Option<i32>> { | ||
| let provides = provides.map(|v| serde_json::to_value(v).unwrap_or_default()); | ||
| diesel::update( | ||
| packages::table | ||
| .filter(packages::repo_name.eq(repo_name)) | ||
| .filter(packages::pkg_name.eq(pkg_name)) | ||
| .filter(packages::pkg_id.eq(pkg_id)) | ||
| .filter(packages::pinned.eq(false)) | ||
| .filter(packages::version.eq(version)), | ||
| ) | ||
| .set(( | ||
| packages::size.eq(size), | ||
| packages::installed_date.eq(installed_date), | ||
| packages::is_installed.eq(true), | ||
| packages::provides.eq(provides), | ||
| packages::with_pkg_id.eq(with_pkg_id), | ||
| packages::checksum.eq(checksum), | ||
| )) | ||
| .returning(packages::id) | ||
| .get_result(conn) | ||
| .optional() | ||
| } |
There was a problem hiding this comment.
Critical: Handle JSON serialization failure explicitly.
Line 381 uses unwrap_or_default() on JSON serialization, which silently converts failures to null. This can result in data loss if PackageProvide fails to serialize. Consider returning a Result or logging the error.
🔎 Proposed fix to propagate serialization error
- pub fn record_installation(
+ pub fn record_installation(
conn: &mut SqliteConnection,
repo_name: &str,
pkg_name: &str,
pkg_id: &str,
version: &str,
size: i64,
provides: Option<Vec<PackageProvide>>,
with_pkg_id: bool,
checksum: Option<&str>,
installed_date: &str,
- ) -> QueryResult<Option<i32>> {
- let provides = provides.map(|v| serde_json::to_value(v).unwrap_or_default());
+ ) -> Result<Option<i32>, Box<dyn std::error::Error>> {
+ let provides = provides
+ .map(|v| serde_json::to_value(v))
+ .transpose()?;
diesel::update(
packages::table
.filter(packages::repo_name.eq(repo_name))
.filter(packages::pkg_name.eq(pkg_name))
.filter(packages::pkg_id.eq(pkg_id))
.filter(packages::pinned.eq(false))
.filter(packages::version.eq(version)),
)
.set((
packages::size.eq(size),
packages::installed_date.eq(installed_date),
packages::is_installed.eq(true),
packages::provides.eq(provides),
packages::with_pkg_id.eq(with_pkg_id),
packages::checksum.eq(checksum),
))
.returning(packages::id)
.get_result(conn)
- .optional()
+ .optional()
+ .map_err(Into::into)
}Alternatively, if you want to keep the current signature, at minimum log the serialization error before defaulting.
📝 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.
| pub fn record_installation( | |
| conn: &mut SqliteConnection, | |
| repo_name: &str, | |
| pkg_name: &str, | |
| pkg_id: &str, | |
| version: &str, | |
| size: i64, | |
| provides: Option<Vec<PackageProvide>>, | |
| with_pkg_id: bool, | |
| checksum: Option<&str>, | |
| installed_date: &str, | |
| ) -> QueryResult<Option<i32>> { | |
| let provides = provides.map(|v| serde_json::to_value(v).unwrap_or_default()); | |
| diesel::update( | |
| packages::table | |
| .filter(packages::repo_name.eq(repo_name)) | |
| .filter(packages::pkg_name.eq(pkg_name)) | |
| .filter(packages::pkg_id.eq(pkg_id)) | |
| .filter(packages::pinned.eq(false)) | |
| .filter(packages::version.eq(version)), | |
| ) | |
| .set(( | |
| packages::size.eq(size), | |
| packages::installed_date.eq(installed_date), | |
| packages::is_installed.eq(true), | |
| packages::provides.eq(provides), | |
| packages::with_pkg_id.eq(with_pkg_id), | |
| packages::checksum.eq(checksum), | |
| )) | |
| .returning(packages::id) | |
| .get_result(conn) | |
| .optional() | |
| } | |
| pub fn record_installation( | |
| conn: &mut SqliteConnection, | |
| repo_name: &str, | |
| pkg_name: &str, | |
| pkg_id: &str, | |
| version: &str, | |
| size: i64, | |
| provides: Option<Vec<PackageProvide>>, | |
| with_pkg_id: bool, | |
| checksum: Option<&str>, | |
| installed_date: &str, | |
| ) -> Result<Option<i32>, Box<dyn std::error::Error>> { | |
| let provides = provides | |
| .map(|v| serde_json::to_value(v)) | |
| .transpose()?; | |
| diesel::update( | |
| packages::table | |
| .filter(packages::repo_name.eq(repo_name)) | |
| .filter(packages::pkg_name.eq(pkg_name)) | |
| .filter(packages::pkg_id.eq(pkg_id)) | |
| .filter(packages::pinned.eq(false)) | |
| .filter(packages::version.eq(version)), | |
| ) | |
| .set(( | |
| packages::size.eq(size), | |
| packages::installed_date.eq(installed_date), | |
| packages::is_installed.eq(true), | |
| packages::provides.eq(provides), | |
| packages::with_pkg_id.eq(with_pkg_id), | |
| packages::checksum.eq(checksum), | |
| )) | |
| .returning(packages::id) | |
| .get_result(conn) | |
| .optional() | |
| .map_err(Into::into) | |
| } |
🤖 Prompt for AI Agents
crates/soar-db/src/repository/core.rs lines 369-401: the call to
serde_json::to_value(...).unwrap_or_default() silently swallows serialization
errors; replace the unwrap_or_default with explicit error propagation by mapping
the serde_json::Error into the function's QueryResult error type (e.g. convert
to diesel::result::Error::SerializationError or otherwise return Err(...))
before performing the diesel::update so the call returns an Err on serialization
failure; alternatively, if you must keep the same signature, log the serde_json
error and return an appropriate diesel::result::Error instead of defaulting to
null.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/soar-cli/src/utils.rs (1)
244-250: Relative path "SOAR_SYMS" may resolve incorrectly.
Path::new("SOAR_SYMS")creates a relative path that depends on the current working directory. If the CWD differs from expectations, this could check the wrong location. Consider making it relative toinstall_dir:🔎 Proposed fix
- let soar_syms = Path::new("SOAR_SYMS"); + let soar_syms = install_dir.join("SOAR_SYMS"); let (is_syms, binaries_dir) = if soar_syms.is_dir() { - (true, soar_syms) + (true, soar_syms.as_path()) } else { (false, install_dir) };
♻️ Duplicate comments (4)
crates/soar-core/src/package/remove.rs (1)
94-98: Potential integer truncation when castingpackage.idfromu64toi32.The cast
self.package.id as i32silently truncates if the ID exceedsi32::MAX. This was flagged in a previous review and remains unaddressed.🔎 Recommended fix using try_into()
- let package_id = self.package.id as i32; + let package_id: i32 = self.package.id.try_into() + .map_err(|_| crate::SoarError::Custom(format!("Package ID {} exceeds i32 range", self.package.id)))?; self.db.transaction(|conn| {crates/soar-cli/src/inspect.rs (1)
111-116: Potential panic on chainedunwrap()calls.The
.unwrap()on line 113 will panic ifselect_package_interactivelyreturnsOk(None)(e.g., user provides invalid input). This was flagged in a previous review and remains unaddressed.🔎 Suggested fix with graceful handling
let selected_pkg = if packages.len() > 1 { - &select_package_interactively(packages, &query.name.unwrap_or(package.to_string()))? - .unwrap() + match select_package_interactively(packages, &query.name.clone().unwrap_or(package.to_string()))? { + Some(pkg) => pkg, + None => { + error!("No package selected"); + return Ok(()); + } + } } else { - packages.first().unwrap() + packages.into_iter().next().unwrap() // safe: len checked above };Note: The
packages.first().unwrap()on line 115 is safe because we reach this branch only whenpackages.len() == 1.crates/soar-cli/src/install.rs (1)
229-237: Potential panic if user cancels interactive selection.The
select_package_interactivelyreturns anOptionthat is unwrapped on line 234. If the user cancels the selection, this will panic.crates/soar-db/src/repository/core.rs (1)
381-382: JSON serialization error is silently converted to null.The
unwrap_or_default()on line 382 silently converts serialization failures tonull, potentially causing data loss. WhilePackageProvideserialization is unlikely to fail, this masks any issues.
🧹 Nitpick comments (16)
crates/soar-db/src/connection.rs (4)
32-34: Consider stricter path validation.Using
to_string_lossy()may silently replace invalid UTF-8 sequences in the path with replacement characters, which could lead to attempting to open an incorrect database file. If database paths are expected to be valid UTF-8, consider usingto_str()and returning an error for invalid UTF-8, or document this behavior explicitly.🔎 Proposed alternative using stricter validation
- let path_str = path.as_ref().to_string_lossy(); + let path_str = path + .as_ref() + .to_str() + .ok_or_else(|| ConnectionError::BadConnection("Invalid UTF-8 in database path".into()))?; let mut conn = SqliteConnection::establish(&path_str)?;
36-49: Error context is lost in conversion to BadConnection.Mapping all migration and query errors to
ConnectionError::BadConnection(e.to_string())loses the original error type information, making it harder to diagnose issues or handle specific error cases in calling code. While this approach provides a consistent error type, consider whether preserving more error context (e.g., via custom error types or error chaining) would be valuable for debugging migration or WAL pragma failures.
67-84: WAL mode not enabled for metadata databases.Similar to
open_without_migrations, this method doesn't enable WAL mode. If concurrent access to metadata databases is expected, WAL mode should be enabled here as well for consistency with theopenmethod's "better concurrent access" goal.🔎 Proposed fix to enable WAL mode
pub fn open_metadata<P: AsRef<Path>>(path: P) -> Result<Self, ConnectionError> { let path_str = path.as_ref().to_string_lossy(); let mut conn = SqliteConnection::establish(&path_str)?; + // WAL mode for better concurrent access + sql_query("PRAGMA journal_mode = WAL;") + .execute(&mut conn) + .map_err(|e| ConnectionError::BadConnection(e.to_string()))?; + // Migrate text JSON to JSONB binary format migrate_json_to_jsonb(&mut conn, DbType::Metadata) .map_err(|e| ConnectionError::BadConnection(e.to_string()))?;
113-127: Documentation example may be misleading.The example uses
ignoreand shows method calls likelist_installed()andsearch()that don't appear to be defined onDbConnection. Consider usingno_runinstead ofignoreand ensuring the example code accurately reflects the actual API, or simplify it to show only connection management operations.crates/soar-db/src/repository/nest.rs (1)
61-65: Consider the check-then-act pattern implications for exists_by_name.The
exists_by_namemethod is correctly implemented. However, if it's used in a check-then-insert pattern (e.g., "if not exists, insert"), be aware that there's a potential TOCTOU (time-of-check to time-of-use) race condition without proper transaction isolation or unique constraints.This is addressed if the database schema enforces uniqueness on the name field, which would cause the insert to fail with a constraint violation instead of silently creating duplicates.
crates/soar-cli/src/state.rs (2)
31-49: Consider extracting duplicated database file removal logic.The
handle_json_metadatafunction removes an existing metadata file before opening a new connection. This pattern is reasonable, but the error mapping uses different styles (with_contextfor fs operations vs manualmap_errfor db operations). Consider using consistent error handling patterns.
241-270: Consider logging errors instead of silently ignoring them.Multiple
if let Ok(...)patterns silently swallow errors when loading repository metadata (lines 245, 253-255). While graceful degradation is good, silent failures can make debugging difficult when repositories fail to load.🔎 Proposed approach
Consider logging warnings when errors occur:
- if let Ok(mut nests_db) = get_nests_db_conn() { - if let Ok(nests) = NestRepository::list_all(nests_db.conn()) { + match get_nests_db_conn() { + Ok(mut nests_db) => { + match NestRepository::list_all(nests_db.conn()) { + Ok(nests) => { + // existing logic + } + Err(e) => tracing::warn!("Failed to list nests: {e}"), + } + } + Err(e) => tracing::warn!("Failed to connect to nests database: {e}"), + }crates/soar-db/src/error.rs (1)
56-66: Generic "Record not found" message may lose context.The
diesel::result::Error::NotFoundis mapped to a generic message. Consider whether callers should wrap this error with more context about which entity was not found, or if this is acceptable as a low-level error.crates/soar-cli/src/use.rs (2)
42-68: Consider simplifying the linked/unlinked status indicator.The logic on lines 63-67 is a bit convoluted with the double negation pattern using
.then()and.unwrap_or_else().🔎 Suggested simplification
- package - .unlinked - .then(String::new) - .unwrap_or_else(|| format!(" {}", Colored(Red, "*"))) + if package.unlinked { String::new() } else { format!(" {}", Colored(Red, "*")) }
88-89: Discarding the result ofmangle_package_symlinksmay hide failures.The
let _ =pattern explicitly discards both the success value and any details about what was mangled. If symlink creation fails silently here, the user might not realize the package isn't fully linked.Consider logging a warning if the operation succeeds but returns unexpected results, or remove
let _ =if the return value isn't needed (the?already propagates errors).🔎 Proposed fix
- let _ = mangle_package_symlinks(&install_dir, &bin_dir, selected_package.provides.as_deref()) - .await?; + mangle_package_symlinks(&install_dir, &bin_dir, selected_package.provides.as_deref()) + .await?;crates/soar-cli/src/health.rs (1)
116-143: TheRc<RefCell<Vec>>pattern is overly complex for this use case.Since
walk_dirtakes&mut Fand the closures don't escape the function, you can use a simpler mutableVecdirectly. The current approach withRc::try_unwrapand fallback cloning adds unnecessary complexity.🔎 Simplified implementation
fn get_broken_symlinks() -> SoarResult<Vec<std::path::PathBuf>> { - let broken_symlinks = Rc::new(RefCell::new(Vec::new())); - - let broken_symlinks_clone = Rc::clone(&broken_symlinks); - let mut collect_action = |path: &Path| -> FileSystemResult<()> { - if !path.exists() { - broken_symlinks_clone.borrow_mut().push(path.to_path_buf()); - } - Ok(()) - }; - - let mut soar_files_action = |path: &Path| -> FileSystemResult<()> { - if let Some(filename) = path.file_stem().and_then(|s| s.to_str()) { - if filename.ends_with("-soar") && !path.exists() { - broken_symlinks_clone.borrow_mut().push(path.to_path_buf()); - } - } - Ok(()) - }; - - walk_dir(&get_config().get_bin_path()?, &mut collect_action)?; - walk_dir(desktop_dir(), &mut soar_files_action)?; - walk_dir(icons_dir(), &mut soar_files_action)?; - - Ok(Rc::try_unwrap(broken_symlinks) - .unwrap_or_else(|rc| rc.borrow().clone().into()) - .into_inner()) + let mut broken_symlinks = Vec::new(); + + let mut collect_action = |path: &Path| -> FileSystemResult<()> { + if !path.exists() { + broken_symlinks.push(path.to_path_buf()); + } + Ok(()) + }; + walk_dir(&get_config().get_bin_path()?, &mut collect_action)?; + + let mut soar_files_action = |path: &Path| -> FileSystemResult<()> { + if let Some(filename) = path.file_stem().and_then(|s| s.to_str()) { + if filename.ends_with("-soar") && !path.exists() { + broken_symlinks.push(path.to_path_buf()); + } + } + Ok(()) + }; + walk_dir(desktop_dir(), &mut soar_files_action)?; + walk_dir(icons_dir(), &mut soar_files_action)?; + + Ok(broken_symlinks) }Note: The closures need to be defined before each
walk_dircall sincecollect_actionborrowsbroken_symlinksmutably and must end beforesoar_files_actionis defined.crates/soar-cli/src/update.rs (3)
82-122: Consider extracting duplicated update target creation logic.The logic for finding newer versions and creating
InstallTargetstructs is nearly identical in both branches (lines 82-122 and 131-171). Consider extracting this into a helper function to reduce duplication.🔎 Suggested helper function
fn create_update_target( pkg: &InstalledPackage, new_pkg: Package, diesel_db: &DieselDatabase, ) -> SoarResult<Option<InstallTarget>> { let existing_install = get_existing(&new_pkg, diesel_db)?; if let Some(ref existing) = existing_install { if existing.is_installed { return Ok(None); } } Ok(Some(InstallTarget { package: new_pkg, existing_install, with_pkg_id: pkg.with_pkg_id, pinned: pkg.pinned, profile: Some(pkg.profile.clone()), portable: pkg.portable_path.clone(), portable_home: pkg.portable_home.clone(), portable_config: pkg.portable_config.clone(), portable_share: pkg.portable_share.clone(), portable_cache: pkg.portable_cache.clone(), })) }Also applies to: 131-171
340-342: Consider logging errors fromremove_old_versionsinstead of silently ignoring.The
let _ =pattern silently discards any errors fromremove_old_versions. While failing to clean up old versions shouldn't block the update, logging these errors would aid debugging orphaned files.🔎 Proposed fix
if !keep { - let _ = remove_old_versions(&target.package, &diesel_db); + if let Err(e) = remove_old_versions(&target.package, &diesel_db) { + tracing::warn!("Failed to remove old versions: {e}"); + } }Apply similarly to line 354.
Also applies to: 353-355
299-299: Potential panic if semaphore is closed.
acquire_owned().await.unwrap()will panic if the semaphore is closed. While unlikely in normal operation, consider usingexpectwith a descriptive message or handling the error gracefully.🔎 Proposed fix
- let permit = ctx.semaphore.clone().acquire_owned().await.unwrap(); + let permit = ctx.semaphore.clone().acquire_owned().await + .expect("semaphore should not be closed during update");crates/soar-cli/src/install.rs (2)
183-222: Repeated metadata query patterns could be consolidated.The pattern for querying metadata (with repo-specific vs. all-repos logic) is repeated multiple times. Consider extracting a helper to reduce duplication and improve maintainability.
Also applies to: 262-301
593-593: Same semaphore unwrap concern as in update.rs.Consider using
expectwith a descriptive message for consistency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
crates/soar-cli/src/health.rscrates/soar-cli/src/inspect.rscrates/soar-cli/src/install.rscrates/soar-cli/src/progress.rscrates/soar-cli/src/state.rscrates/soar-cli/src/update.rscrates/soar-cli/src/use.rscrates/soar-cli/src/utils.rscrates/soar-core/src/error.rscrates/soar-core/src/package/remove.rscrates/soar-core/src/package/update.rscrates/soar-db/src/connection.rscrates/soar-db/src/error.rscrates/soar-db/src/repository/core.rscrates/soar-db/src/repository/metadata.rscrates/soar-db/src/repository/nest.rscrates/soar-utils/src/error.rscrates/soar-utils/src/fs.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/soar-utils/src/fs.rs
- crates/soar-core/src/package/update.rs
🧰 Additional context used
🧬 Code graph analysis (10)
crates/soar-db/src/error.rs (5)
crates/soar-db/src/repository/core.rs (1)
from(54-80)crates/soar-core/src/error.rs (1)
from(161-163)crates/soar-utils/src/error.rs (1)
from(273-339)crates/soar-core/src/database/models.rs (4)
from(146-172)from(177-203)from(208-264)from(269-273)crates/soar-db/src/repository/metadata.rs (1)
diesel(199-201)
crates/soar-cli/src/utils.rs (2)
crates/soar-config/src/config.rs (2)
get_config(123-135)display(414-416)crates/soar-utils/src/fs.rs (1)
is_elf(306-311)
crates/soar-cli/src/health.rs (6)
crates/soar-config/src/config.rs (2)
get_config(123-135)display(414-416)crates/soar-core/src/package/remove.rs (1)
remove(30-101)crates/soar-utils/src/fs.rs (1)
walk_dir(172-206)crates/soar-utils/src/path.rs (2)
desktop_dir(141-143)icons_dir(146-148)crates/soar-db/src/repository/core.rs (1)
list_broken(147-157)crates/soar-registry/src/package.rs (1)
s(32-32)
crates/soar-db/src/connection.rs (1)
crates/soar-db/src/migration.rs (2)
apply_migrations(25-40)migrate_json_to_jsonb(73-147)
crates/soar-utils/src/error.rs (1)
crates/soar-utils/src/fs.rs (1)
create_symlink(121-142)
crates/soar-cli/src/progress.rs (2)
crates/soar-config/src/config.rs (1)
display(414-416)crates/soar-cli/src/utils.rs (1)
display_settings(51-53)
crates/soar-core/src/package/remove.rs (2)
crates/soar-core/src/package/install.rs (1)
new(60-113)crates/soar-db/src/repository/core.rs (2)
delete_portable(524-527)delete(459-461)
crates/soar-db/src/repository/nest.rs (2)
crates/soar-db/src/connection.rs (2)
nests(203-205)conn(87-89)crates/soar-db/src/repository/core.rs (5)
list_all(88-90)find_by_id(256-262)find_by_name(280-285)insert(350-355)delete(459-461)
crates/soar-cli/src/inspect.rs (6)
crates/soar-cli/src/utils.rs (3)
display_settings(51-53)interactive_ask(63-76)select_package_interactively(107-125)crates/soar-cli/src/progress.rs (1)
create_spinner(54-71)crates/soar-db/src/repository/core.rs (2)
find_exact(174-192)from(54-80)crates/soar-cli/src/state.rs (1)
new(63-73)crates/soar-core/src/package/query.rs (1)
try_from(20-67)crates/soar-db/src/repository/metadata.rs (1)
find_filtered(301-335)
crates/soar-db/src/repository/metadata.rs (2)
crates/soar-db/src/models/metadata.rs (1)
new(140-145)crates/soar-db/src/models/types.rs (1)
from_string(29-55)
⏰ 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: test
- GitHub Check: coverage
🔇 Additional comments (58)
crates/soar-db/src/connection.rs (4)
1-14: LGTM!The module documentation clearly describes the database types and their purposes. Imports are appropriate for the functionality provided.
86-104: LGTM!The accessor methods and
Deref/DerefMuttrait implementations provide flexible and ergonomic access to the underlyingSqliteConnection. This is a standard Rust pattern for wrapper types.
137-163: LGTM!The
newmethod correctly initializes the core and nests database connections with proper migration support. The metadata HashMap starting empty is appropriate since repositories are added dynamically.
165-216: LGTM with a note on metadata DB replacement behavior.The API design is clean and provides flexible access to database connections. Note that
add_metadata_dbwill silently replace any existing metadata database with the same repository name due toHashMap::insertbehavior. If this is intended, consider documenting it; otherwise, you might want to return the old connection or error on duplicate names.crates/soar-db/src/repository/nest.rs (6)
1-8: LGTM!The module documentation and imports are clear and appropriate for a Diesel-based repository.
10-11: LGTM!Using a unit struct for namespacing repository methods is an idiomatic Rust pattern and consistent with the codebase.
14-26: LGTM!Both methods follow standard Diesel query patterns and are consistent with the CoreRepository implementation.
51-59: LGTM!Both delete methods follow standard Diesel patterns and are consistent with the CoreRepository implementation.
13-66: Add update methods to NestRepository for complete CRUD support, or document why nests are immutable.Diesel supports CRUD operations which include "Create Read Update Delete". The repository currently lacks update functionality. If nests are intentionally immutable after creation, this should be explicitly documented. Otherwise, callers requiring modifications would need to delete and re-insert, which is inefficient and doesn't preserve the original ID.
28-44: Thenameandurlfields have UNIQUE constraints defined at the column level in the database schema, making theOption<Nest>return type correct and the implementation safe.crates/soar-cli/src/state.rs (4)
51-73: LGTM!Clean use of
Arc<AppStateInner>withOnceCellfor lazy initialization of database connections. The pattern allows thread-safe sharing while deferring expensive operations.
136-175: LGTM!The parallel metadata fetch with sequential result processing is appropriate. Error handling correctly distinguishes between task join failures and metadata fetch failures.
177-229: LGTM!Good implementation of package replacement detection. The logic properly checks if an installed package still exists in metadata and attempts to find and apply replacements when needed.
89-99: The id cast from i32 to i64 is safe and requires no changes.The
nest.idfield is an i32, and casting i32 to i64 is a safe widening conversion that cannot truncate or misrepresent values. No issues exist here.Likely an incorrect or invalid review comment.
crates/soar-db/src/error.rs (2)
6-54: LGTM!Well-designed error enum with comprehensive diagnostic annotations. Each variant has appropriate error codes and helpful user-facing guidance. The
#[from]attribute onIoErrorsimplifies conversions.
68-75: LGTM!Standard pattern for connection error conversion and result type alias.
crates/soar-core/src/error.rs (3)
10-138: LGTM!Excellent enhancement to error handling with diagnostic metadata. The
#[diagnostic(transparent)]for wrapped errors and explicit codes/help for other variants provide good user-facing diagnostics. The pattern is consistent across all variants.
140-157: LGTM!The
root_causemethod provides appropriate unwrapping for errors that have meaningful source chains.
166-185: LGTM!Clean implementation of the
ErrorContexttrait. This provides an ergonomic way to add action context to IO errors, similar to patterns in anyhow/eyre.crates/soar-utils/src/error.rs (5)
8-64: LGTM!Well-structured error types with comprehensive diagnostic annotations. The
BytesError,HashError, andPathErrorenums provide clear, actionable error messages with appropriate help text.
66-191: LGTM!Comprehensive
FileSystemErrorenum covering all common filesystem operations. Good distinction between IO errors (with#[source]) and validation errors (NotFound,NotADirectory,NotAFile) that don't wrap underlying errors.
193-270: LGTM!Clean implementation of
IoContextandIoOperation. The helper constructors provide an ergonomic API for creating context, andcreate_symlinkcorrectly captures both source and target paths.
272-354: LGTM!The
Fromimplementation correctly maps eachIoOperationvariant to the correspondingFileSystemError. TheIoResultExttrait provides a clean extension method for adding path context to IO results.
378-443: LGTM!Tests properly verify the error message formatting and have been updated to match the new single-quoted path format.
crates/soar-cli/src/utils.rs (2)
28-59: LGTM!Clean utility additions. The
Iconsstruct provides centralized icon constants, and the helper functions (icon_or,display_settings,term_width) are simple and focused. The fallback terminal width of 80 is a reasonable default.
291-309: LGTM!Good validation logic with a clear, actionable error message that lists valid repository options for the current platform.
crates/soar-cli/src/progress.rs (4)
5-14: LGTM: Clean import organization and spinner constant.The new imports for
ConfigProgressStyleanddisplay_settingsare appropriately added, and theSPINNER_CHARSconstant provides a nice set of Unicode spinner frames for visual feedback.
16-21: LGTM: Good refactor to delegate style creation.Extracting the style selection logic to
get_progress_style()improves separation of concerns and makescreate_progress_bar()cleaner.
23-52: LGTM: Well-structured progress style selection.The
get_progress_style()function cleanly maps configuration options to distinct visual styles. Each variant is appropriately configured with its own template, progress characters, and the sharedformat_byteskey.
54-71: LGTM: Good spinner implementation with configuration awareness.The
create_spinnerfunction respects thespinners()display setting, providing a graceful fallback to a plain message template when spinners are disabled. The tick duration of 80ms provides smooth animation.crates/soar-cli/src/use.rs (3)
1-17: LGTM: Appropriate imports for the use-alternate flow.The imports cover all necessary dependencies for database access, package models, integration utilities, and user interaction.
120-142: Good fix: Empty package check now guardsintegrate_package.The condition
!pkg.is_empty() && pkg.iter().all(has_desktop_integration)correctly prevents the vacuous truth issue from the previous review. The logic flow between desktop integration and portable setup is clear.
144-158: LGTM: Transaction-wrapped link operation with clear success logging.The link operation is properly wrapped in a transaction, and the success message provides clear feedback about which package was activated.
crates/soar-core/src/package/remove.rs (2)
8-12: LGTM: Clean import updates for Diesel migration.The imports correctly bring in
DieselDatabase,CoreRepository, andProvideStrategyto support the new repository-based deletion flow.
17-28: LGTM: Constructor updated for DieselDatabase.The struct and constructor properly accept
DieselDatabaseinstead of the previousArc<Mutex<Connection>>, aligning with the Diesel migration.crates/soar-cli/src/inspect.rs (4)
1-22: LGTM: Updated imports for Diesel-based access and spinner utilities.The imports correctly bring in the new repository types, spinner utilities, and display settings for the refactored implementation.
38-58: LGTM: Clean migration to CoreRepository for installed path lookup.The function properly uses
DieselDatabase.with_conn()andCoreRepository::find_exact()to query the installed package, with appropriate handling of the optional result.
67-106: LGTM: Well-structured metadata query with repo-aware mapping.The branching logic correctly handles both repo-specific and global queries, properly mapping the results to
Packageobjects with enrichedrepo_namefields. Thequery_all_flatpattern for cross-repo queries is clean.
165-177: LGTM: Good spinner integration with configuration awareness.The spinner is conditionally created based on display settings and properly cleared after the fetch completes. The
Optionpattern handles the disabled case cleanly.crates/soar-cli/src/health.rs (3)
1-21: LGTM: Comprehensive imports for health monitoring.The imports cover all necessary dependencies for table rendering, filesystem operations, configuration access, and styled output.
23-102: LGTM: Well-structured health display with clear remediation guidance.The
display_health()function provides a clean overview of system state with actionable commands for fixing issues. The table formatting is consistent and the conditional detail sections are helpful.
145-168: LGTM: Clean broken package removal flow.The function properly iterates over broken packages, removes each one using
PackageRemover, and provides clear logging. The.clone()ondiesel_dbis appropriate since it's passed to eachPackageRemoverinstance.crates/soar-cli/src/update.rs (3)
1-28: LGTM on import reorganization.The imports are properly structured for the Diesel-based refactoring, correctly importing
DieselDatabase, repository types, and UI utilities.
30-45: LGTM onget_existingfunction.The function correctly uses
CoreRepository::find_exactthrough theDieselDatabaseconnection wrapper and properly handles the optional result conversion.
200-289: LGTM onperform_updatefunction.The async orchestration, error collection, and summary display logic are well-structured. The icon-enabled table rendering provides good UX enhancement.
crates/soar-cli/src/install.rs (3)
377-456: LGTM onselect_packagefunction.The function properly handles all cases: no packages found, single package (auto-select), and multiple packages (interactive or auto-select with
yesflag).
458-584: LGTM onperform_installationfunction.The installation orchestration, progress tracking, and summary display are well-implemented. The icon-enabled table output provides consistent UX with the update flow.
643-888: LGTM oninstall_single_packagefunction.The function properly handles existing installations, signature verification, checksum validation, and integration. Error handling is appropriate throughout.
crates/soar-db/src/repository/metadata.rs (5)
1-23: LGTM on module structure and imports.The use of
OnceLockfor theMAINTAINER_REregex is the correct approach for lazy static initialization without external dependencies.
340-367: LGTM onfind_newer_versionwith corrected indexing.The Rust slice
[14..]and SQLsubstr(version, 15)now correctly align to extract the same substring from HEAD-prefixed versions. The version comparison logic is sound.
377-399: LGTM onimport_packagestransactional import.The transaction ensures atomicity - either all packages are imported or none. The empty etag on upsert is appropriate as it will be updated separately after successful import.
461-479: LGTM on conflict handling and maintainer linking.The early return on
inserted == 0correctly prevents callinglast_insert_id()when no row was inserted due to conflict. Maintainer linking only occurs for newly inserted packages.
485-496: LGTM onextract_name_and_contactwith cached regex.The
OnceLockpattern correctly initializes the regex once and reuses it for all subsequent calls, addressing the previous performance concern.crates/soar-db/src/repository/core.rs (5)
248-253: Improved distinct count with null byte separator.Using
'\x00'(null byte) as a separator is a good improvement over simple concatenation. This character is extremely unlikely to appear inpkg_idorpkg_name, effectively eliminating collision risk.
25-81: LGTM onInstalledPackageWithPortablestruct and conversion.The struct properly combines package data with optional portable configuration. The
Fromimplementation correctly handles the optional portable fields.
86-171: LGTM on list functions with portable joins.The
list_filtered,list_broken, andlist_updatablefunctions correctly useleft_jointo include optional portable configuration. The filtering and sorting logic is well-implemented.
485-521: LGTM onupsert_portableimplementation.The update-then-insert pattern is a valid approach for SQLite upsert when the conflict columns differ from the update columns. The logic correctly falls back to insert when no row is updated.
529-589: LGTM on old package cleanup functions.Both
get_old_package_pathsanddelete_old_packagescorrectly identify the latest package by ID and exclude it from cleanup. The pinned filter ensures user-pinned packages are preserved.
| /// Inserts a new nest. | ||
| pub fn insert(conn: &mut SqliteConnection, nest: &NewNest) -> QueryResult<usize> { | ||
| diesel::insert_into(nests::table).values(nest).execute(conn) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Align insert return type with CoreRepository for API consistency.
The insert method returns QueryResult<usize> (row count), while CoreRepository::insert returns QueryResult<i32> (the inserted ID). Returning the ID is generally more useful and maintains consistency across repository interfaces.
🔎 Proposed fix to return the inserted ID
- /// Inserts a new nest.
- pub fn insert(conn: &mut SqliteConnection, nest: &NewNest) -> QueryResult<usize> {
- diesel::insert_into(nests::table).values(nest).execute(conn)
+ /// Inserts a new nest and returns its ID.
+ pub fn insert(conn: &mut SqliteConnection, nest: &NewNest) -> QueryResult<i32> {
+ diesel::insert_into(nests::table)
+ .values(nest)
+ .returning(nests::id)
+ .get_result(conn)
}🤖 Prompt for AI Agents
In crates/soar-db/src/repository/nest.rs around lines 46-49, change the insert
function to return the inserted row ID (QueryResult<i32>) instead of the
affected row count: update the signature to pub fn insert(conn: &mut
SqliteConnection, nest: &NewNest) -> QueryResult<i32>, perform the insert as
before, then fetch and return the last inserted row id using Diesel's
last_insert_rowid (e.g. run the insert, then
diesel::select(diesel::dsl::last_insert_rowid).get_result::<i64>(conn) and cast
to i32) — wrap both operations in a transaction or propagate errors so you
return QueryResult<i32> of the inserted ID.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
crates/soar-db/src/migration.rs (2)
141-144: Wrap migration queries in a transaction for atomicity.The function executes multiple UPDATE queries sequentially without a transaction boundary. If a query fails mid-way (e.g., due to malformed JSON), earlier queries have already committed, leaving some columns migrated while others remain as text, creating an inconsistent database state.
🔎 Proposed fix: Add transaction boundary
+use diesel::connection::Connection; + pub fn migrate_json_to_jsonb( conn: &mut SqliteConnection, db_type: DbType, ) -> Result<usize, Box<dyn Error + Send + Sync + 'static>> { + conn.transaction(|conn| { // Check for text type OR blob containing text JSON (starts with '[' or '{') // Use hex comparison for blobs: 5B = '[', 7B = '{' let json_condition = |col: &str| { format!( "{col} IS NOT NULL AND (typeof({col}) = 'text' OR (typeof({col}) = 'blob' AND hex(substr({col}, 1, 1)) IN ('5B', '7B')))" ) }; // ... query building logic ... let mut total = 0; for query in queries { total += sql_query(&query).execute(conn)?; } Ok(total) + }) }
141-144: Add context to migration errors for easier debugging.If a query fails (e.g., due to invalid JSON), the error provides no indication of which column or query caused the failure. When migrating 9+ columns, this makes troubleshooting difficult.
🔎 Proposed improvement: Add error context
let mut total = 0; - for query in queries { - total += sql_query(&query).execute(conn)?; + for (idx, query) in queries.iter().enumerate() { + total += sql_query(query) + .execute(conn) + .map_err(|e| format!("Migration query #{} failed: {}\nQuery: {}", idx, e, query))?; }crates/soar-cli/src/state.rs (1)
316-328: Previous race condition concern is addressed.The
metadata_managermethod now correctly usestokio::sync::OnceCellwith the initialization logic (init_repo_dbs,sync_nests,create_metadata_manager) inside theget_or_try_initasync closure. This ensures only one task performs initialization while others await.crates/soar-cli/src/install.rs (1)
230-238: Potential panic if user cancels package selection.The code unwraps the
Optionreturned byselect_package_interactively, which could panic if the user cancels the selection. This was flagged in a previous review but remains unaddressed.🔎 Proposed fix
let pkg = if repo_pkgs.len() > 1 { - &select_package_interactively( + match select_package_interactively( repo_pkgs, &query.name.unwrap_or(package.clone()), - )? - .unwrap() + )? { + Some(pkg) => pkg, + None => continue, // User cancelled selection + } } else { repo_pkgs.first().unwrap() };
🧹 Nitpick comments (10)
crates/soar-utils/src/error.rs (2)
193-270: Clean abstraction for IO operation context.The
IoContextandIoOperationtypes provide excellent structure for capturing filesystem operation context. The comprehensive set of constructor methods makes the API ergonomic and type-safe.💡 Optional: Consider adding a `path()` accessor
While the private fields provide good encapsulation, a public
path()getter (similar to the existingoperation()getter) could be useful for debugging or logging purposes:impl IoContext { // ... existing methods ... pub fn operation(&self) -> &IoOperation { &self.operation } + + pub fn path(&self) -> &Path { + &self.path + } }
382-447: Tests verify core error message formatting.The test suite covers key error variants and confirms the display messages are correct. While the derives handle most functionality automatically, the tests provide confidence in the expected user-facing messages.
💡 Optional: Consider expanding test coverage
For more comprehensive validation, you could add tests for:
- Diagnostic codes using
miette::Diagnostictrait methods- Help messages
- Error source chaining (verify
std::error::Error::source()returns the expected nested error)UtilsErrorconversions via?operator or.into()However, since the derives handle most of this automatically, the current tests are adequate.
crates/soar-db/src/connection.rs (1)
55-70: Clarify documentation foropen_without_migrations()regarding JSON migration.The method docs mention skipping schema migrations but don't explicitly state that
migrate_json_to_jsonb()is also skipped. Since the codebase providesopen_metadata()specifically for handling JSON migration separately, consider updating the documentation to clarify that callers usingopen_without_migrations()must ensure the database is fully migrated (both schema and JSON conversion already applied). Alternatively, if this method is intended for databases where JSON columns may still be in text format, JSON migration should be performed as well.crates/soar-cli/src/health.rs (2)
116-143: Simplify:Rc<RefCell>is unnecessary here.The
Rc<RefCell>pattern adds complexity without benefit since this function is synchronous and single-threaded. A simple mutableVecwould suffice.Additionally, the symlink detection logic may be incorrect:
!path.exists()returnsfalsefor a symlink whose target doesn't exist (broken symlink), butwalk_diriterates over directory entries, not symlink targets. Consider usingpath.symlink_metadata()to check if it's a symlink, thenpath.exists()to check if the target exists.🔎 Proposed simplification
fn get_broken_symlinks() -> SoarResult<Vec<std::path::PathBuf>> { - let broken_symlinks = Rc::new(RefCell::new(Vec::new())); - - let broken_symlinks_clone = Rc::clone(&broken_symlinks); - let mut collect_action = |path: &Path| -> FileSystemResult<()> { - if !path.exists() { - broken_symlinks_clone.borrow_mut().push(path.to_path_buf()); + let mut broken_symlinks = Vec::new(); + + let mut collect_action = |path: &Path| -> FileSystemResult<()> { + // Check if it's a symlink with a non-existent target + if path.symlink_metadata().map(|m| m.file_type().is_symlink()).unwrap_or(false) + && !path.exists() + { + broken_symlinks.push(path.to_path_buf()); } Ok(()) }; - let mut soar_files_action = |path: &Path| -> FileSystemResult<()> { + let mut soar_files_action = |path: &Path| -> FileSystemResult<()> { if let Some(filename) = path.file_stem().and_then(|s| s.to_str()) { - if filename.ends_with("-soar") && !path.exists() { - broken_symlinks_clone.borrow_mut().push(path.to_path_buf()); + if filename.ends_with("-soar") + && path.symlink_metadata().map(|m| m.file_type().is_symlink()).unwrap_or(false) + && !path.exists() + { + broken_symlinks.push(path.to_path_buf()); } } Ok(()) }; walk_dir(&get_config().get_bin_path()?, &mut collect_action)?; walk_dir(desktop_dir(), &mut soar_files_action)?; walk_dir(icons_dir(), &mut soar_files_action)?; - Ok(Rc::try_unwrap(broken_symlinks) - .unwrap_or_else(|rc| rc.borrow().clone().into()) - .into_inner()) + Ok(broken_symlinks) }
104-114: Consider acceptingAppStateas a parameter to avoid redundant initialization.Both
get_broken_packagesandremove_broken_packagescreate a newAppStateinstance. If these functions are called afterdisplay_health(which likely has its ownAppState), this results in redundant initialization of database connections.Consider refactoring to accept
&AppStateas a parameter for reuse:-async fn get_broken_packages() -> SoarResult<Vec<(String, String, String)>> { - let state = AppState::new(); +async fn get_broken_packages(state: &AppState) -> SoarResult<Vec<(String, String, String)>> { let diesel_db = state.diesel_core_db()?;Also applies to: 145-147
crates/soar-cli/src/state.rs (2)
254-268: Consider logging errors instead of silently ignoring them.The nested
if let Ok(...)patterns silently swallow errors fromget_nests_db_conn(),NestRepository::list_all(), andget_repositories_path(). This can make debugging difficult when nest metadata fails to load.Consider logging at debug/warn level to aid troubleshooting:
- if let Ok(mut nests_db) = get_nests_db_conn() { - if let Ok(nests) = NestRepository::list_all(nests_db.conn()) { + match get_nests_db_conn() { + Ok(mut nests_db) => match NestRepository::list_all(nests_db.conn()) { + Ok(nests) => { // ... existing logic ... + } + Err(e) => tracing::debug!("Failed to list nests: {e}"), + }, + Err(e) => tracing::debug!("Failed to open nests DB: {e}"), + }
32-50: Consider atomic file replacement for metadata database.The current pattern removes the existing file before creating the new one. If the process crashes between removal and successful write, the metadata file will be missing. For improved robustness, consider writing to a temporary file first, then renaming:
let temp_path = metadata_db.with_extension("db.tmp"); // ... write to temp_path ... fs::rename(&temp_path, metadata_db)?;This is a minor concern since metadata can be re-synced, but worth considering for reliability.
crates/soar-cli/src/list.rs (2)
60-66: Consider filtering installed packages query for better performance.The code fetches all installed packages into a HashMap to check installation status. For users with many installed packages, this could be inefficient. Consider filtering by the package keys that are actually in the search results, or using a more targeted query.
534-592: Complex fold pattern is functional but could benefit from clarity.The fold accumulates multiple counters and conditionally updates them based on installation status. While correct, consider extracting the per-package processing into a helper function for improved readability.
🔎 Suggested refactor for clarity
+ struct PackageStats { + installed_count: u32, + unique_count: u32, + broken_count: u32, + installed_size: u64, + broken_size: u64, + } + let (installed_count, unique_count, broken_count, installed_size, broken_size) = - packages.iter().fold( - (0, 0, 0, 0, 0), - |(installed_count, unique_count, broken_count, installed_size, broken_size), - package| { - // ... complex logic - }, - ); + packages.iter().fold( + PackageStats::default(), + |mut stats, package| { + // Update stats... + stats + }, + );crates/soar-cli/src/update.rs (1)
340-355: Consider logging cleanup failures instead of silently ignoring.Both occurrences of
remove_old_versionsuselet _ =to discard the result, which silently ignores any cleanup failures. While this shouldn't block the update, logging a warning would aid debugging.🔎 Proposed improvement
if !keep { - let _ = remove_old_versions(&target.package, &diesel_db); + if let Err(e) = remove_old_versions(&target.package, &diesel_db) { + warn!("Failed to remove old versions: {}", e); + } }Apply similarly to line 354.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.tomlcrates/soar-cli/Cargo.tomlcrates/soar-cli/src/health.rscrates/soar-cli/src/install.rscrates/soar-cli/src/list.rscrates/soar-cli/src/state.rscrates/soar-cli/src/update.rscrates/soar-core/Cargo.tomlcrates/soar-db/src/connection.rscrates/soar-db/src/migration.rscrates/soar-package/Cargo.tomlcrates/soar-utils/src/error.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/soar-cli/Cargo.toml
🧰 Additional context used
🧬 Code graph analysis (5)
crates/soar-cli/src/list.rs (6)
crates/soar-cli/src/state.rs (2)
config(274-276)new(64-74)crates/soar-utils/src/fs.rs (1)
dir_size(263-284)crates/soar-cli/src/utils.rs (4)
icon_or(43-49)pretty_package_size(134-139)term_width(55-59)vec_string(93-95)crates/soar-core/src/database/models.rs (12)
repo_name(93-95)repo_name(139-141)pkg_name(81-83)pkg_name(127-129)pkg_id(85-87)pkg_id(131-133)version(89-91)version(135-137)from(146-172)from(177-203)from(208-264)from(269-273)crates/soar-db/src/repository/metadata.rs (4)
search_case_sensitive(166-187)search(141-163)find_filtered(301-335)count(206-208)crates/soar-db/src/repository/core.rs (4)
list_filtered(94-144)count(229-231)count_distinct_installed(234-253)from(54-80)
crates/soar-cli/src/update.rs (4)
crates/soar-core/src/package/update.rs (1)
remove_old_versions(15-38)crates/soar-db/src/repository/core.rs (3)
find_exact(174-192)list_filtered(94-144)list_updatable(160-171)crates/soar-core/src/package/remove.rs (1)
new(23-28)crates/soar-db/src/repository/metadata.rs (1)
find_newer_version(340-367)
crates/soar-db/src/connection.rs (1)
crates/soar-db/src/migration.rs (3)
sql_query(48-49)apply_migrations(25-40)migrate_json_to_jsonb(73-147)
crates/soar-cli/src/health.rs (6)
crates/soar-cli/src/state.rs (2)
config(274-276)new(64-74)crates/soar-config/src/config.rs (3)
get_config(123-135)new(238-256)display(414-416)crates/soar-utils/src/fs.rs (1)
walk_dir(172-206)crates/soar-utils/src/path.rs (2)
desktop_dir(141-143)icons_dir(146-148)crates/soar-cli/src/utils.rs (2)
icon_or(43-49)term_width(55-59)crates/soar-db/src/repository/core.rs (1)
list_broken(147-157)
crates/soar-utils/src/error.rs (1)
crates/soar-utils/src/fs.rs (1)
create_symlink(121-142)
⏰ 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: coverage
- GitHub Check: test
🔇 Additional comments (27)
crates/soar-utils/src/error.rs (7)
1-17: Excellent use ofthiserrorandmiettefor structured error handling.The derive-based approach with diagnostic codes and contextual help messages significantly improves error reporting quality. The
BytesErrorimplementation is clean and provides actionable guidance.
19-32: LGTM! Proper error chaining with#[source].The use of
#[source]ensures the underlyingstd::io::Erroris preserved in the error chain, enabling full diagnostic context.
34-64: Well-designedPathErrorwith comprehensive variant coverage.The four variants cover common path operation failures with appropriate diagnostic codes and helpful guidance. The distinction between validation errors (Empty, UnclosedVariable) and runtime errors (FailedToGetCurrentDir, MissingEnvVar) is clear.
66-191: ComprehensiveFileSystemErrorwith rich diagnostic metadata.The enum covers all major filesystem operations with consistent error messages and helpful guidance. The special handling of
CreateSymlinkwith separatefromandtargetfields provides clear context when symlink creation fails.
272-340: LGTM! Exhaustive and correct error conversion.The
Fromimplementation properly maps eachIoOperationvariant to its correspondingFileSystemError, preserving both the path context and the underlyingstd::io::Error. The special handling ofCreateSymlinkcorrectly captures both the source path and symlink target.
342-354: Excellent extension trait for ergonomic error handling.The
IoResultExttrait enables clean, chainable error context attachment:std::fs::operation().with_path(path, operation)?. This pattern significantly improves code readability at call sites.
356-380: Perfect!HashErrorvariant properly integrated.The
UtilsErrorenum now correctly includes all error types (Bytes, Path, FileSystem, Hash) with transparent diagnostic forwarding. The#[from]attributes enable seamless error conversion throughout the utils crate. This addresses the concern from the previous review.crates/soar-db/src/migration.rs (1)
10-10: LGTM: Appropriate derives for the enum.The
CloneandCopyderives are suitable for this simple enum, making it more ergonomic to pass around without borrowing.crates/soar-db/src/connection.rs (4)
32-53: LGTM: Well-structured initialization with proper migration handling.The method correctly:
- Establishes the connection
- Enables WAL mode for concurrency
- Applies schema migrations
- Selectively runs JSON-to-JSONB migration only for Core and Nest databases
The conditional migration (lines 43-48) appropriately excludes Metadata databases, which use the dedicated
open_metadatamethod instead.
72-94: LGTM: Appropriate metadata database handling.This method correctly:
- Enables WAL mode for concurrent access
- Skips schema migrations (as documented, since metadata schemas are externally managed)
- Performs JSON-to-JSONB migration to handle externally-generated databases
The approach aligns well with the documented use case of metadata databases generated by rusqlite.
102-114: LGTM: Standard wrapper pattern.The
DerefandDerefMutimplementations allowDbConnectionto be used transparently as aSqliteConnection, which is a common and appropriate pattern for wrapper types.
116-226: LGTM: Well-designed database orchestration.The
DatabaseManagerprovides a clean API for managing multiple database connections:
- Clear separation between core, metadata, and nests databases
- Appropriate use of
HashMapfor per-repository metadata connections- Good ergonomics with iterator methods (
all_metadata,metadata_names)- Comprehensive documentation with usage examples
The design makes it easy to manage the lifecycle of multiple database connections.
crates/soar-package/Cargo.toml (1)
14-14: LGTM - minor patch version bump.The image crate update from 0.25.8 to 0.25.9 is a routine patch version bump. Features remain unchanged.
Cargo.toml (2)
63-67: Verify the opt-level change from3to"z"is intentional.Changing
opt-levelfrom3(maximum speed) to"z"(minimum size) prioritizes binary size reduction over runtime performance. This can noticeably slow down hot paths, especially in CPU-bound operations like package processing or compression.If binary size is a priority (e.g., for distribution), this is appropriate. Otherwise, consider keeping
opt-level = 3or using"s"for a balanced approach.
3-12: LGTM - clean workspace reorganization.The workspace member paths and path dependencies are correctly updated to reflect the
crates/directory structure. This improves project organization.Also applies to: 45-52
crates/soar-cli/src/health.rs (1)
23-102: LGTM - well-structured health check display.The function provides clear, actionable output with color-coded status indicators and remediation guidance. The table rendering with
tabledis appropriate for this use case.crates/soar-core/Cargo.toml (1)
13-29: LGTM - clean migration to workspace-managed dependencies.The shift to
{ workspace = true }for shared dependencies ensures version consistency across crates. The addition ofsoar-dbaligns with the Diesel-based repository pattern introduced in this PR.crates/soar-cli/src/state.rs (1)
76-176: LGTM - well-structured async sync with concurrent fetching.The pattern of spawning tasks for parallel metadata fetches and then awaiting/processing results is appropriate. Error handling for join errors and fetch failures is properly logged.
crates/soar-cli/src/list.rs (2)
170-373: LGTM!The
query_packagefunction is well-structured with proper handling of repo-specific vs. cross-repo queries, comprehensive metadata display, and good use of optional field handling. The structured logging captures all relevant package details.
380-403: Good improvement: Usinglist_allinstead of hardcoded pagination.The previous hardcoded pagination limit of 3000 has been replaced with
MetadataRepository::list_all, which properly retrieves all packages without arbitrary limits.crates/soar-cli/src/update.rs (3)
30-45: LGTM!The
get_existingfunction is cleanly migrated to use Diesel-basedCoreRepository::find_exact. The parameter ordering matches the repository signature and the result is properly converted.
236-286: LGTM!The update summary rendering properly handles both icon and text modes, with appropriate handling of the edge case where no packages were updated and none failed.
82-97: Verifyfind_newer_versioncorrectly handles HEAD version comparisons.The implementation in
find_newer_version(lines 347-357 of crates/soar-db/src/repository/metadata.rs) correctly handles HEAD-prefixed versions by extracting a suffix starting at position 14 of the version string and using it for lexicographic comparison. The query filters for either regular versions greater than the current version, or HEAD versions with a greater suffix. The logic is sound and aligns with the expected versioning scheme.crates/soar-cli/src/install.rs (4)
86-99: LGTM!The
create_install_contextfunction properly initializes the progress bar with conditional styling based on display settings.
378-457: LGTM!The
select_packagefunction properly handles all three lookup cases (existing install, repo-specific, all repos) and correctly returnsOption<Package>with appropriate handling for 0, 1, or multiple results.
499-530: LGTM!The per-package installation output is well-formatted with clear binary paths, notes display, and proper icon usage based on display settings.
644-889: LGTM!The
install_single_packagefunction is comprehensive with proper error handling for:
- Directory cleanup before installation
- Signature verification with minisign
- Checksum validation with appropriate warnings
- Desktop integration and package recording
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.