feat(crates): init soar-config crate #108
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughExtracts configuration into a new crate Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as soar-cli
participant CORE as soar-core
participant CONFIG as soar-config
participant DB as nests.db (sqlite)
rect rgb(247,250,255)
note left of CLI: Flow after refactor (high-level)
CLI->>CONFIG: get_config()
CONFIG-->>CLI: Config
CLI->>CORE: call feature (pass Config)
CORE->>CONFIG: query profiles/repos
CORE->>DB: get_nests_db_conn(&Config) (runs migrations)
DB-->>CORE: DB Connection
CORE-->>CLI: result/status
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas needing extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
soar-core/src/package/formats/common.rs (1)
103-135: Avoidunwrap()onget_config().get_bin_path()insymlink_desktop
symlink_desktopcurrently does:let final_content = { let re = Regex::new(r"(?m)^(Icon|Exec|TryExec)=(.*)").unwrap(); re.replace_all(&content, |caps: ®ex::Captures| { match &caps[1] { "Icon" => format!("Icon={}-soar", file_name.to_string_lossy()), "Exec" | "TryExec" => { let value = &caps[0]; let bin_path = get_config().get_bin_path().unwrap(); let new_value = format!("{}/{}", &bin_path.display(), pkg_name); …If
get_bin_path()fails (bad config, IO issues, etc.), this will panic instead of returning aSoarErrorlike the rest of the function. It also recomputes the config/bin path for every matched line.You can compute the bin path once and propagate errors via
?:@@ - let final_content = { - let re = Regex::new(r"(?m)^(Icon|Exec|TryExec)=(.*)").unwrap(); - - re.replace_all(&content, |caps: ®ex::Captures| { + let bin_path = get_config().get_bin_path()?; + let bin_path = bin_path.display().to_string(); + + let final_content = { + let re = Regex::new(r"(?m)^(Icon|Exec|TryExec)=(.*)").unwrap(); + + re.replace_all(&content, |caps: ®ex::Captures| { match &caps[1] { "Icon" => format!("Icon={}-soar", file_name.to_string_lossy()), "Exec" | "TryExec" => { let value = &caps[0]; - let bin_path = get_config().get_bin_path().unwrap(); - let new_value = format!("{}/{}", &bin_path.display(), pkg_name); + let new_value = format!("{}/{}", bin_path, pkg_name); @@ } _ => unreachable!(), } }) .to_string() };This removes the panic path and avoids repeated config lookups inside the regex callback.
crates/soar-config/src/config.rs (1)
52-55: repositories_path documentation does not match actual default pathThe comment on
Config::repositories_pathsays:Default: $SOAR_ROOT/packages
But both the default config and profile helpers use a
reposdirectory:
default_configsetsrepositories_path: Some(format!("{soar_root}/repos")).Profile::get_repositories_pathreturnsroot_path.join("repos").get_repositories_pathultimately falls back todefault_profile()?.get_repositories_path().So the effective default is
$SOAR_ROOT/repos, not$SOAR_ROOT/packages. Suggest updating the doc comment to avoid confusion, e.g.:- /// Path to the local clone of all repositories. - /// Default: $SOAR_ROOT/packages + /// Path to the local clone of all repositories. + /// Default: $SOAR_ROOT/reposAlso applies to: 215-216, 367-375
🧹 Nitpick comments (4)
soar-core/src/utils.rs (1)
92-99: Connection reopening pattern after migration.The function opens a connection, runs migrations (which consumes the connection), then reopens it. This pattern is necessary because
migration::run_neststakes ownership of the connection. However, consider if there's value in returning the connection from the migration function instead to avoid the double open.The error mapping to
SoarError::Customwith a format string is acceptable for now, though you may want to consider adding a more specific error variant for migration failures in the future for better error handling granularity.soar-core/src/error.rs (1)
3-11: Consider consolidating config-related error variantsWiring
SoarError::Config(#[from] ConfigError)tosoar_config::error::ConfigErroris correct, butSoarErrorstill definesConfigAlreadyExistsandInvalidProfilevariants which conceptually overlap withConfigError::ConfigAlreadyExistsandConfigError::InvalidProfile. Having two parallel representations for the same conditions can be confusing and lead to inconsistent handling.It’s worth considering:
- Routing all configuration failures through
ConfigError(viaSoarError::Config) and- Gradually deprecating or removing the overlapping
SoarErrorvariants if they’re no longer needed.Also applies to: 75-86, 115-116
crates/soar-config/src/profile.rs (1)
5-7: Simplify error conversion and align with Config’s resolve_path usageThese methods currently convert
PathError -> UtilsError -> ConfigErrorviamap_err(UtilsError::from)?, whileConfigdirectly relies onFrom<PathError> for ConfigErrorby usingresolve_path(..)?. You can simplify and make this consistent:-use soar_utils::{error::UtilsError, path::resolve_path}; - -use crate::error::ConfigError; +use soar_utils::path::resolve_path; + +use crate::error::ConfigError; @@ pub fn get_packages_path(&self) -> Result<PathBuf, ConfigError> { if let Some(ref packages_path) = self.packages_path { - Ok(resolve_path(packages_path).map_err(UtilsError::from)?) + Ok(resolve_path(packages_path)?) } else { Ok(self.get_root_path()?.join("packages")) } } @@ pub fn get_root_path(&self) -> Result<PathBuf, ConfigError> { if let Ok(env_path) = std::env::var("SOAR_ROOT") { - return Ok(resolve_path(&env_path).map_err(UtilsError::from)?); + return Ok(resolve_path(&env_path)?); } - Ok(resolve_path(&self.root_path).map_err(UtilsError::from)?) + Ok(resolve_path(&self.root_path)?) }This drops an unnecessary conversion layer and mirrors how
Config::get_*_pathalready handlesresolve_path.Also applies to: 23-57
crates/soar-config/src/config.rs (1)
388-395: Use self instead of get_config() and consider reusing sync-interval parsing
get_nests_sync_intervalis an instance method but ignoresselfand instead readsget_config().nests_sync_interval. This couples it to the global singleton and makes the result depend on global state even if you call it on a standaloneConfiginstance.You can make it purely instance-based and also keep behavior aligned with
Repository::sync_interval:- pub fn get_nests_sync_interval(&self) -> u128 { - match get_config().nests_sync_interval.as_deref().unwrap_or("3h") { + pub fn get_nests_sync_interval(&self) -> u128 { + match self.nests_sync_interval.as_deref().unwrap_or("3h") { "always" => 0, "never" => u128::MAX, "auto" => 3 * 3_600_000, - value => parse_duration(value).unwrap_or(3_600_000), + value => parse_duration(value).unwrap_or(3_600_000), } }If you end up adjusting the error fallback or supported keywords for repo sync intervals, consider extracting a small helper so
Repository::sync_intervalandget_nests_sync_intervalcan share the same logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
Cargo.toml(3 hunks)crates/soar-config/Cargo.toml(1 hunks)crates/soar-config/src/annotations.rs(4 hunks)crates/soar-config/src/config.rs(9 hunks)crates/soar-config/src/error.rs(1 hunks)crates/soar-config/src/lib.rs(1 hunks)crates/soar-config/src/profile.rs(1 hunks)crates/soar-config/src/repository.rs(1 hunks)crates/soar-config/src/utils.rs(1 hunks)crates/soar-utils/src/fs.rs(7 hunks)soar-cli/Cargo.toml(1 hunks)soar-cli/src/download.rs(1 hunks)soar-cli/src/health.rs(1 hunks)soar-cli/src/install.rs(1 hunks)soar-cli/src/list.rs(1 hunks)soar-cli/src/main.rs(1 hunks)soar-cli/src/nest.rs(3 hunks)soar-cli/src/state.rs(4 hunks)soar-cli/src/use.rs(1 hunks)soar-cli/src/utils.rs(1 hunks)soar-core/Cargo.toml(1 hunks)soar-core/src/error.rs(1 hunks)soar-core/src/lib.rs(0 hunks)soar-core/src/metadata.rs(1 hunks)soar-core/src/package/formats/common.rs(4 hunks)soar-core/src/package/install.rs(1 hunks)soar-core/src/package/remove.rs(1 hunks)soar-core/src/utils.rs(3 hunks)
💤 Files with no reviewable changes (1)
- soar-core/src/lib.rs
🧰 Additional context used
🧬 Code graph analysis (18)
soar-cli/src/use.rs (2)
soar-cli/src/state.rs (1)
config(249-251)crates/soar-config/src/config.rs (1)
get_config(121-133)
soar-core/src/package/install.rs (1)
crates/soar-config/src/config.rs (1)
get_config(121-133)
soar-cli/src/list.rs (2)
soar-cli/src/state.rs (1)
config(249-251)crates/soar-config/src/config.rs (1)
get_config(121-133)
crates/soar-config/src/lib.rs (1)
soar-cli/src/state.rs (1)
config(249-251)
soar-cli/src/health.rs (1)
crates/soar-config/src/config.rs (1)
get_config(121-133)
soar-cli/src/download.rs (2)
soar-cli/src/state.rs (1)
config(249-251)crates/soar-config/src/config.rs (1)
get_config(121-133)
soar-cli/src/utils.rs (2)
crates/soar-config/src/config.rs (1)
get_config(121-133)crates/soar-config/src/repository.rs (1)
get_platform_repositories(86-151)
soar-core/src/utils.rs (3)
soar-cli/src/state.rs (1)
config(249-251)crates/soar-config/src/config.rs (2)
get_config(121-133)Config(117-117)soar-core/src/database/migration.rs (1)
run_nests(112-116)
crates/soar-config/src/profile.rs (3)
crates/soar-utils/src/path.rs (1)
resolve_path(41-62)crates/soar-config/src/config.rs (6)
get_bin_path(329-337)get_db_path(339-347)get_packages_path(349-355)get_cache_path(357-365)get_repositories_path(367-375)get_portable_dirs(377-386)crates/soar-config/src/error.rs (3)
from(100-102)from(106-108)from(112-114)
soar-core/src/package/formats/common.rs (2)
crates/soar-config/src/config.rs (1)
get_config(121-133)crates/soar-utils/src/fs.rs (1)
walk_dir(165-199)
crates/soar-config/src/repository.rs (2)
crates/soar-utils/src/time.rs (1)
parse_duration(23-54)crates/soar-config/src/config.rs (1)
get_config(121-133)
soar-cli/src/nest.rs (2)
crates/soar-config/src/config.rs (1)
get_config(121-133)soar-core/src/utils.rs (1)
get_nests_db_conn(92-99)
soar-core/src/package/remove.rs (1)
crates/soar-config/src/config.rs (1)
get_config(121-133)
crates/soar-config/src/annotations.rs (1)
crates/soar-config/src/config.rs (3)
annotate_toml_table(428-428)annotate_toml_table(434-434)annotate_toml_array_of_tables(442-442)
soar-cli/src/main.rs (2)
soar-cli/src/state.rs (1)
config(249-251)crates/soar-config/src/config.rs (4)
generate_default_config(450-472)get_config(121-133)set_current_profile(142-150)Config(117-117)
soar-cli/src/install.rs (2)
crates/soar-config/src/config.rs (1)
get_config(121-133)crates/soar-config/src/utils.rs (1)
default_install_patterns(1-6)
soar-cli/src/state.rs (2)
crates/soar-config/src/config.rs (2)
get_config(121-133)Config(117-117)soar-core/src/utils.rs (1)
get_nests_db_conn(92-99)
crates/soar-config/src/config.rs (6)
crates/soar-utils/src/path.rs (3)
resolve_path(41-62)xdg_config_home(96-100)xdg_data_home(115-119)crates/soar-utils/src/system.rs (1)
platform(9-16)crates/soar-utils/src/time.rs (1)
parse_duration(23-54)crates/soar-config/src/annotations.rs (3)
annotate_toml_array_of_tables(106-115)annotate_toml_table(55-95)annotate_toml_table(111-111)crates/soar-config/src/repository.rs (1)
get_platform_repositories(86-151)crates/soar-config/src/profile.rs (6)
get_bin_path(24-26)get_db_path(28-30)get_packages_path(32-38)get_cache_path(40-42)get_repositories_path(44-46)get_portable_dirs(48-50)
⏰ 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 (25)
soar-core/Cargo.toml (1)
17-17: LGTM! Dependencies added to support configuration refactoring.The addition of
mietteandsoar-configworkspace dependencies appropriately supports the extraction of configuration management into the newsoar-configcrate.Also applies to: 23-23
crates/soar-config/Cargo.toml (1)
1-20: LGTM! Well-structured manifest for the new configuration crate.The manifest appropriately declares workspace-level metadata and dependencies for the new
soar-configcrate. The dependency set (documented, miette, serde, soar-utils, thiserror, toml, toml_edit, tracing) is suitable for configuration management functionality.Cargo.toml (1)
3-3: LGTM! Workspace configuration updated correctly.The workspace members and dependencies have been properly expanded to include the new
soar-configcrate and its required dependencies (documented, toml, toml_edit). The path-based dependency forsoar-configcorrectly referencescrates/soar-config.Also applies to: 28-28, 42-42, 48-49
soar-core/src/utils.rs (1)
6-7: LGTM! Import updates align with configuration refactoring.The imports have been properly updated to use
soar_config::configfor configuration-related types, and new imports forrusqlite::Connectionanddatabase::migrationsupport the newget_nests_db_connfunction.Also applies to: 16-19
crates/soar-utils/src/fs.rs (2)
165-199: Good refactor: Improved flexibility with generic error types.The updated signature allows callers to use custom error types as long as
FileSystemError: Into<E>. This is a solid design improvement that provides better flexibility for error handling across different contexts.Note: This is a breaking API change. Ensure all call sites throughout the codebase have been updated to match the new signature (the test updates in this file demonstrate the pattern).
483-483: LGTM! Test closures updated correctly.The test closures have been properly annotated with explicit return types (
-> FileSystemResult<()>) to match the new generic signature ofwalk_dir.Also applies to: 498-498, 516-516, 548-550, 561-561, 576-576
soar-cli/src/use.rs (1)
6-6: LGTM! Import path updated correctly.The import has been properly updated to use
soar_config::config::get_config, aligning with the configuration refactoring that moves configuration management to the newsoar-configcrate.soar-cli/src/download.rs (1)
5-5: LGTM! Import path updated correctly.The import has been properly updated to use
soar_config::config::get_config, consistent with the configuration refactoring effort.soar-cli/Cargo.toml (1)
34-34: LGTM! Dependency added to support configuration refactoring.The
soar-configworkspace dependency has been appropriately added to enable the CLI to use the newly extracted configuration crate.soar-cli/src/health.rs (1)
4-4: LGTM! Import path correctly updated.The import path has been successfully migrated from
soar_core::configtosoar_config::config, aligning with the new crate structure. The usage ofget_config()throughout the file remains unchanged and consistent.soar-core/src/package/remove.rs (1)
9-9: LGTM! Configuration access correctly migrated to soar-config.The import has been properly updated from the local
crate::configto the newsoar_config::configmodule, maintaining the same functionality while supporting the crate extraction.soar-cli/src/list.rs (1)
9-9: LGTM! Import path correctly updated.The configuration import has been successfully migrated to the new
soar_configcrate, consistent with the broader refactoring effort.soar-core/src/package/install.rs (1)
10-10: LGTM! Configuration import successfully migrated.The import has been properly updated from the local config module to the centralized
soar_config::configmodule, with all usages remaining consistent.soar-cli/src/install.rs (1)
17-17: LGTM! Configuration and utility imports correctly migrated.Both
get_configanddefault_install_patternshave been successfully relocated fromsoar_coreto the newsoar_configcrate, centralizing configuration management as intended by this refactoring.soar-cli/src/utils.rs (1)
14-14: LGTM! Configuration and repository imports correctly updated.The imports for
get_configandget_platform_repositorieshave been successfully migrated to the newsoar_configcrate, maintaining consistency with the overall refactoring effort.crates/soar-config/src/utils.rs (1)
1-6: LGTM! Clean implementation of default install patterns.The function provides a clear set of default exclusion patterns for package installation. The implementation is straightforward and correctly returns the patterns as a
Vec<String>.soar-core/src/metadata.rs (1)
8-8: LGTM! Repository type correctly migrated to soar-config.The import has been properly updated to use
Repositoryfromsoar_config::repositoryinstead of the local config module. This changes the public signature offetch_metadata, but maintains the same functionality and is consistent with the broader crate extraction effort.soar-cli/src/main.rs (1)
15-17: Config imports correctly migrated tosoar-configThe new
soar_config::configimports line up with all config usages in this file (init,generate_default_config,get_config,set_current_profile,CONFIG_PATH,Config) and keep error propagation viaSoarError::Configintact. Looks good.soar-cli/src/nest.rs (1)
1-12: Nest DB access correctly switched to free function + config crateUsing
soar_config::config::get_config()combined withsoar_core::utils::get_nests_db_conn(&config)?is consistent with the newget_nests_db_conn(&Config)signature and keeps the behavior ofadd_nest/remove_nest/list_nestsunchanged.Also applies to: 24-27, 34-37
crates/soar-config/src/lib.rs (1)
1-6: Public module surface forsoar-configlooks appropriateRe-exporting
annotations,config,error,profile,repository, andutilsas public modules cleanly exposes the configuration API for other crates.soar-core/src/package/formats/common.rs (1)
94-99: Improved symlink handling and error propagation look good
- Pre-removing existing symlinks before creating icon/desktop links avoids EEXIST errors and gives clearer diagnostics when removal fails.
- Changing the
walk_dircallbacks to returnSoarResult<()>integrates nicely withwalk_dir’s genericEand existingFileSystemError -> SoarErrorconversions, so symlink failures now bubble up cleanly throughintegrate_package.This is a solid reliability improvement for package integration.
Also applies to: 146-151, 248-269
soar-cli/src/state.rs (1)
10-13: AppState correctly migrated tosoar-configtypes and nest DB util
- Using
soar_config::config::{get_config, Config}inAppStateInnerandAppState::newkeeps configuration handling centralized in the new crate.- Switching from a method-style call to
get_nests_db_conn(&self.config())?matches the new utility signature while preserving the existingsync_nestsandcreate_repo_dbbehavior.- Updating
validate_packagesto take&soar_config::repository::Repositoryintegrates smoothly with the new repository type;repo.get_path()?will convertConfigErrorintoSoarError::Configas expected.No issues spotted here.
Also applies to: 43-53, 61-68, 129-135, 204-223, 248-251
crates/soar-config/src/repository.rs (1)
10-46: Repository struct and basic helpers look consistentThe struct layout, derives, and
get_path/is_enabledbehavior line up with howConfigandProfileexpose repository paths and defaults.get_pathcorrectly anchors underConfig::get_repositories_path()andis_enabled’sunwrap_or(true)matches the documented default.crates/soar-config/src/annotations.rs (1)
7-8: Standardized ConfigError/Result usage in annotations looks goodUsing the shared
Resultalias and dedicatedConfigErrorvariants (UnexpectedTomlItem,AnnotateFirstTable) makes these helpers much clearer and keeps error handling consistent across the crate. The control flow and logging behavior are unchanged, so this is a safe cleanup.Also applies to: 55-95, 106-115
crates/soar-config/src/config.rs (1)
422-447: to_annotated_document wiring with annotations/profile/repository is cleanSerializing to TOML, reparsing into
DocumentMut, and then annotating:
- Root table via
annotate_toml_table::<Config>,- Each profile table via
annotate_toml_table::<Profile>,- The repositories array via
annotate_toml_array_of_tables::<Repository>,is a straightforward and maintainable way to keep generated configs in sync with struct docs. Localizing the
Itemimport inside the function also keeps the module imports tidy.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/soar-config/src/error.rs (1)
5-97: Normalize diagnosticcode(...)names for repository variantsThe diagnostic
codevalues are almost all in thesoar_config::...namespace, but the two repository-related variants use flat identifiers:
- Line 50:
code(soar_config_invalid_repository)- Line 54:
code(soar_config_invalid_repository_url)For consistency and easier tooling/grepping, consider renaming them along the same pattern as the others, e.g.:
- #[diagnostic(code(soar_config_invalid_repository))] + #[diagnostic(code(soar_config::invalid_repository))] ... - #[diagnostic(code(soar_config_invalid_repository_url))] + #[diagnostic(code(soar_config::invalid_repository_url))]
🧹 Nitpick comments (1)
crates/soar-config/src/error.rs (1)
99-115: Remove redundantFromimpls; leverage automatic conversion chain throughUtilsErrorVerified:
UtilsErroralready hasFrom<PathError>,From<BytesError>, andFrom<FileSystemError>impls insoar-utils, andConfigError::Utilshas the#[from]attribute. The three explicit impls at lines 99–115 are redundant—they manually construct what automatic conversions would handle. You can remove them and let callers convert throughUtilsErrordirectly, or the error will propagate automatically if your code path already uses?operator on UtilsError-producing operations. This reduces coupling and maintenance burden.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/soar-config/src/error.rs(1 hunks)
⏰ 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 (1)
crates/soar-config/src/error.rs (1)
117-117: Crate-wideResultalias looks goodThe
pub type Result<T> = std::result::Result<T, ConfigError>;alias is idiomatic and will keep call sites concise and consistent.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/soar-config/src/config.rs (1)
448-465: Remove redundantcreate_dir_alland avoid potential panic ingenerate_default_config
generate_default_configcurrently callsfs::create_dir_all(config_path.parent().unwrap())?;and then callscreate_dir_allagain in theif let Some(parent)block. The first call is both redundant and risky ifCONFIG_PATHhas no parent (e.g.,SOAR_CONFIGis just a file name), sinceunwrap()would panic.You can safely drop the first call and rely on the second, which already handles the
Nonecase.if config_path.exists() { return Err(ConfigError::ConfigAlreadyExists); } - fs::create_dir_all(config_path.parent().unwrap())?; - let def_config = Config::default_config(external, repos); let annotated_doc = def_config.to_annotated_document()?; if let Some(parent) = config_path.parent() { fs::create_dir_all(parent)?;
🧹 Nitpick comments (2)
crates/soar-config/src/config.rs (2)
315-319: Avoidunreachable!()indefault_profileand return a real errorRight now a missing default profile will panic despite the function returning
Result. It’s safer and more self-consistent to returnConfigError::MissingDefaultProfilehere, even thoughresolve()should maintain the invariant.- pub fn default_profile(&self) -> Result<&Profile> { - self.profile - .get(&self.default_profile) - .ok_or_else(|| unreachable!()) - } + pub fn default_profile(&self) -> Result<&Profile> { + self.profile + .get(&self.default_profile) + .ok_or_else(|| ConfigError::MissingDefaultProfile(self.default_profile.clone())) + }
386-393:get_nests_sync_intervalignoresselfand always reads the global configFor consistency with other getters and for easier testing of
Configinstances in isolation, consider usingself.nests_sync_intervaldirectly instead of callingget_config()inside this method. That way, a manually constructedConfigwon’t have its ownnests_sync_intervalsilently ignored.- pub fn get_nests_sync_interval(&self) -> u128 { - match get_config().nests_sync_interval.as_deref().unwrap_or("3h") { + pub fn get_nests_sync_interval(&self) -> u128 { + match self.nests_sync_interval.as_deref().unwrap_or("3h") { "always" => 0, "never" => u128::MAX, "auto" => 3 * 3_600_000, value => parse_duration(value).unwrap_or(3_600_000), } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/soar-config/src/config.rs(10 hunks)soar-core/src/package/formats/common.rs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
soar-core/src/package/formats/common.rs (3)
crates/soar-config/src/config.rs (2)
get_config(119-131)new(233-251)crates/soar-utils/src/error.rs (2)
remove_file(343-345)new(324-329)crates/soar-utils/src/fs.rs (1)
walk_dir(165-199)
crates/soar-config/src/config.rs (6)
crates/soar-utils/src/path.rs (3)
resolve_path(41-62)xdg_config_home(96-100)xdg_data_home(115-119)crates/soar-utils/src/system.rs (1)
platform(9-16)crates/soar-utils/src/time.rs (1)
parse_duration(23-54)crates/soar-config/src/annotations.rs (3)
annotate_toml_array_of_tables(106-115)annotate_toml_table(55-95)annotate_toml_table(111-111)crates/soar-config/src/repository.rs (1)
get_platform_repositories(87-152)crates/soar-config/src/profile.rs (6)
get_bin_path(24-26)get_db_path(28-30)get_packages_path(32-38)get_cache_path(40-42)get_repositories_path(44-46)get_portable_dirs(48-50)
🔇 Additional comments (10)
crates/soar-config/src/config.rs (6)
8-24: Imports cleanly align this module with the newsoar-configcrate layoutBringing in serde/doc traits, the crate-local
Resultalias, and movingProfile/Repositoryto their own modules keepsconfig.rsfocused onConfigand shared helpers. This organization looks consistent and clear.
50-56:repositories_pathdefault doc matches the actual initializationThe documented default
$SOAR_ROOT/reposis consistent with the value set indefault_config, which helps avoid config/docs drift.
231-251: Config loading and fallback behavior inConfig::newis soundFalling back to
default_configonly onNotFound, while surfacing other IO and TOML parse errors through the crate’sResulttype, is a sensible and predictable policy for config loading. No changes needed here.
420-445: Annotated TOML generation via serialize → parse → annotate is straightforwardSerializing
Configto TOML, reparsing intoDocumentMut, and then annotating config/profile/repository sections keeps the annotated document aligned with the actual serialized shape, and the localItemimport keeps the module scope tidy. This implementation looks solid.
327-384: Asymmetry in profile fallback behavior is confirmed; developer verification requiredThe code indeed shows an inconsistency: when a user calls
set_current_profile()to switch profiles, the getters behave differently:
get_cache_path()andget_packages_path()respect the switched profile viaget_current_profile()get_bin_path(),get_db_path(),get_repositories_path(), andget_portable_dirs()always use the default profileThis split is real in the codebase and multi-profile workflows are actively supported (evidenced by
set_current_profile()being called in soar-cli/src/main.rs:133). However, no documentation, comments, or tests in the codebase explain whether this asymmetry is intentional or accidental.Please confirm: Is this split by design (some resources should always come from the default profile), or should all path getters consistently follow the current profile?
272-283: I need to check if there are any test configurations or user-facing examples that define repositories with thenest-prefix:The web search results are unrelated to the specific codebase. Let me search the repository for test configurations or examples:Let me check if there are any test configs or fixtures in the repository itself that use the
nest-prefix:Based on my verification, the concern in the original review comment is not valid. The grep output confirms that all
nest-prefixed names are internally generated by the system usingformat!("nest-{}", ...)patterns, not user-defined repository names.The
nest-prefix reservation is a namespace protection mechanism that prevents users from creating repositories with names that would conflict with the system's internal naming scheme. This is not a breaking change because:
- Users never defined repositories with the
nest-prefix (the system generates these names automatically)- The validation prevents future collisions between user-defined and system-generated names
- No existing user configurations are affected
The implementation is correct and requires no changes.
soar-core/src/package/formats/common.rs (4)
11-15: Usingsoar_config::config::get_configmatches the new config crate boundarySwitching to the public
soar-configcrate for configuration access keeps this module decoupled from core-internal config definitions. This aligns well with the new crate layout.
85-101: Idempotent icon symlink creation with pre-removal of existing symlinkRemoving an existing symlink at
final_pathbefore creating a new one avoidsEEXISTerrors and keeps icon integration idempotent. This is a good, localized hardening; no functional issues spotted.
103-154: Desktop integration now respects config-drivenbin_pathand cleans up old symlinksUsing
get_config().get_bin_path()?when rewritingExec/TryExecensures desktop entries follow the configured/overridden bin directory, and the pre-removal of an existing symlink at the final desktop path makes the operation repeatable without manual cleanup. This change looks correct and improves robustness.
249-270: Directory walk closures now propagate symlink errors viaSoarResultChanging the
symlink_actionclosures to returnSoarResult<()>and using?onsymlink_desktop/symlink_iconmakesintegrate_packagefail cleanly on integration errors instead of panicking or silently continuing. This matches the function’sSoarResult<()>signature and is a nice tightening of error handling.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Cargo.toml(3 hunks)crates/soar-config/src/annotations.rs(4 hunks)crates/soar-config/src/config.rs(13 hunks)crates/soar-config/src/error.rs(1 hunks)crates/soar-config/src/lib.rs(1 hunks)crates/soar-config/src/profile.rs(1 hunks)crates/soar-config/src/repository.rs(1 hunks)crates/soar-config/src/test_utils.rs(1 hunks)crates/soar-config/src/utils.rs(1 hunks)soar-core/src/error.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/soar-config/src/utils.rs
- soar-core/src/error.rs
- crates/soar-config/src/annotations.rs
🧰 Additional context used
🧬 Code graph analysis (4)
crates/soar-config/src/repository.rs (2)
crates/soar-utils/src/time.rs (1)
parse_duration(23-54)crates/soar-config/src/config.rs (1)
get_config(119-131)
crates/soar-config/src/config.rs (8)
crates/soar-utils/src/path.rs (3)
resolve_path(41-62)xdg_config_home(96-100)xdg_data_home(115-119)crates/soar-utils/src/system.rs (1)
platform(9-16)crates/soar-utils/src/time.rs (1)
parse_duration(23-54)crates/soar-config/src/annotations.rs (4)
annotate_toml_array_of_tables(106-115)annotate_toml_table(55-95)annotate_toml_table(111-111)Config(155-155)crates/soar-config/src/repository.rs (1)
get_platform_repositories(87-152)crates/soar-config/src/profile.rs (6)
get_bin_path(24-26)get_db_path(28-30)get_packages_path(32-38)get_cache_path(40-42)get_repositories_path(44-46)get_portable_dirs(48-50)crates/soar-config/src/test_utils.rs (1)
with_env(2-23)crates/soar-config/src/error.rs (3)
from(100-102)from(106-108)from(112-114)
crates/soar-config/src/lib.rs (1)
soar-cli/src/state.rs (1)
config(249-251)
crates/soar-config/src/profile.rs (4)
crates/soar-utils/src/path.rs (1)
resolve_path(41-62)crates/soar-config/src/config.rs (6)
get_bin_path(327-335)get_db_path(337-345)get_packages_path(347-353)get_cache_path(355-363)get_repositories_path(365-373)get_portable_dirs(375-384)crates/soar-config/src/test_utils.rs (1)
with_env(2-23)crates/soar-config/src/error.rs (3)
from(100-102)from(106-108)from(112-114)
⏰ 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 (14)
Cargo.toml (2)
3-10: LGTM: Workspace member additionThe new
soar-configcrate is properly integrated into the workspace members.
21-54: LGTM: Dependency declarationsThe workspace dependencies for the new crate (
documented,toml,toml_edit,soar-config) are correctly declared and versioned.crates/soar-config/src/error.rs (2)
5-97: LGTM: Comprehensive error type with diagnosticsThe
ConfigErrorenum is well-structured with appropriate diagnostic codes and help messages. Past review feedback regarding the typo and diagnostic code consistency has been addressed.
99-115: LGTM: Error conversionsThe
Fromimplementations correctly wrap utility errors into theConfigError::Utilsvariant, maintaining the error chain.crates/soar-config/src/lib.rs (1)
1-9: LGTM: Clean module structureThe module declarations are well-organized with appropriate visibility and conditional compilation for test utilities.
crates/soar-config/src/profile.rs (2)
9-58: LGTM: Well-structured Profile with path resolutionThe
Profilestruct correctly implements path resolution withSOAR_ROOTenvironment override. The layering is appropriate, withConfigmethods (in config.rs) providing additional environment overrides likeSOAR_BIN,SOAR_DB, etc.
60-110: LGTM: Comprehensive testsThe tests cover creation, explicit vs. default packages path, and environment override scenarios using the
with_envtest utility.crates/soar-config/src/config.rs (4)
150-229: LGTM: Well-designed default configurationThe
default_configmethod correctly builds a sensible default configuration with platform-aware repository selection and proper fallback values.
253-313: LGTM: Thorough validation logicThe
resolve()method properly validates repository names (including thenest-prefix check on line 278), checks for duplicates, and sets default values. The validation logic is comprehensive and well-tested.
327-384: LGTM: Consistent path resolution patternAll path getter methods follow a consistent pattern: check environment variable first, then config field, then profile default. This provides a clear override hierarchy.
470-680: LGTM: Excellent test coverageThe test suite is comprehensive, covering default config creation, repository selection, validation errors (missing profile, reserved names, nest prefix, duplicates), path overrides, and serialization. This demonstrates high-quality engineering.
crates/soar-config/src/repository.rs (3)
47-56: LGTM: Corrected signature verification logicThe
signature_verification()method now properly short-circuits whenpubkeyisNonebefore applying the global override, preventing the confusing "verification enabled but no key" state. This addresses the previous review feedback.
58-72: LGTM: Corrected sync interval with proper defaultThe
sync_interval()method now uses3 * 3_600_000(3 hours) as the fallback, which correctly matches the documented default of "3h". This addresses the previous review feedback.
87-152: LGTM: Platform-aware repository definitionsThe
get_platform_repositories()function provides a well-structured list of default repositories with appropriate platform support and metadata.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/soar-utils/src/fs.rs (1)
146-183: Genericwalk_dirAPI and error mapping look good; tiny doc clarity nitThe new generic signature
walk_dir<P, F, E>withF: FnMut(&Path) -> Result<(), E>andFileSystemError: Into<E>is coherent, and the use of.map_err(|e| e.into())?cleanly liftsFileSystemErrorinto the caller’s error type while preserving recursive behavior.One minor polish: the
# Errorssection still says “Returns aFileSystemError::Directory/FileSystemError::NotADirectory”, but the public return type is nowResult<(), E>. You might consider rephrasing to something like “Errors originate fromFileSystemError::Directory/FileSystemError::NotADirectory(converted intoEviaInto)” to avoid confusion for callers using a non-FileSystemErrorE.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/soar-utils/src/fs.rs(9 hunks)
⏰ 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 (3)
crates/soar-utils/src/fs.rs (3)
250-255:dir_sizeexample change is correct and idiomaticUsing
dir_size("/tmp/dir").unwrap_or(0)in the example is consistent with theFileSystemResult<u64>return type and demonstrates a simple fallback without changing the API. No issues here.
286-298:is_elfdoc import now matches the public APIThe example’s
use soar_utils::fs::is_elf;import correctly reflects how consumers should pull this function from the crate and keeps the snippet self-contained. Looks good.
471-577:walk_dirtests properly updated for the generic error typeThe tests now explicitly type the callbacks as
-> FileSystemResult<()>, keepingE = FileSystemErrorand ensuring the new generic signature compiles and behaves as before for success, not-a-directory, missing path, and permission-denied (top-level and nested) scenarios. This gives solid coverage of the updated API.
Introduces soar-config crate :)
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.