Add TOML configuration file support #128
Conversation
WalkthroughAdds a TOML-backed configuration subsystem to keep-cli: new dependencies, example config, a typed Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Client (keep CLI)
participant Config as Config module
participant FS as Filesystem
participant Handler as Command dispatch / Handlers
participant Output as Output
CLI->>Config: Config::load()
Config->>FS: read default config path
FS-->>Config: file contents / NotFound
Config->>Config: parse TOML, expand ~, validate relays/timeout
Config-->>CLI: Config instance
CLI->>Handler: dispatch command with &Config
Handler->>Config: default_relay(), vault_path(), timeout_secs()
Handler->>FS: (Init) create dir & write sample config
Handler->>Output: emit results
Output-->>CLI: display response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@keep-cli/src/main.rs`:
- Around line 422-424: The code silently converts a potential error from
cfg.vault_path() into an empty PathBuf via unwrap_or_default(), which yields
confusing failures later; change the logic around let path =
cli.path.unwrap_or_else(|| cfg.vault_path().unwrap_or_default()) to instead
handle the Result from cfg.vault_path() explicitly: if cli.path is Some use it,
otherwise call cfg.vault_path() and propagate or surface any error (return Err
or .expect with a clear message) so failures like missing $HOME are reported
immediately and not converted into an empty path.
🧹 Nitpick comments (2)
keep-cli/src/config.rs (2)
57-71: Consider stricter URL validation.The current validation checks only the scheme prefix. Malformed URLs like
wss://(empty host) orwss://[invalidwould pass validation but fail at runtime when connecting.Consider using the
urlcrate (already a dependency) for full URL parsing:♻️ Optional: Use url::Url for stricter validation
fn deserialize_relays<'de, D>(deserializer: D) -> std::result::Result<Vec<String>, D::Error> where D: Deserializer<'de>, { let relays: Vec<String> = Vec::deserialize(deserializer)?; for relay in &relays { - if !relay.starts_with("wss://") && !relay.starts_with("ws://") { - return Err(serde::de::Error::custom(format!( - "Invalid relay URL: '{}'. Must start with wss:// or ws://", - relay - ))); + let url = url::Url::parse(relay).map_err(|e| { + serde::de::Error::custom(format!("Invalid relay URL '{}': {}", relay, e)) + })?; + if url.scheme() != "ws" && url.scheme() != "wss" { + return Err(serde::de::Error::custom(format!( + "Invalid relay URL: '{}'. Must use ws:// or wss:// scheme", + relay + ))); } } Ok(relays) }
136-140: Minor: Error variant may be misleading.
default_path()returnsKeepError::HomeNotFoundwhendirs::config_dir()returnsNone. This could happen when both$XDG_CONFIG_HOMEand$HOMEare unset. While technically correct (home is needed to derive config dir), a more specific error message could help users diagnose the issue.♻️ Optional: More descriptive error
pub fn default_path() -> Result<PathBuf> { dirs::config_dir() .map(|p| p.join("keep").join("config.toml")) - .ok_or(KeepError::HomeNotFound) + .ok_or_else(|| KeepError::Other( + "Could not determine config directory (HOME or XDG_CONFIG_HOME not set)".into() + )) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
keep-cli/Cargo.tomlkeep-cli/contrib/config.toml.examplekeep-cli/src/config.rskeep-cli/src/main.rskeep-cli/src/output.rs
💤 Files with no reviewable changes (1)
- keep-cli/src/output.rs
🧰 Additional context used
🧬 Code graph analysis (2)
keep-cli/src/config.rs (1)
keep-core/src/lib.rs (1)
default_keep_path(372-376)
keep-cli/src/main.rs (1)
keep-cli/src/config.rs (4)
load(112-119)default_relay(149-154)default_path(136-140)timeout_secs(156-158)
🔇 Additional comments (13)
keep-cli/Cargo.toml (1)
38-38: LGTM!The
tomlandshellexpanddependencies are appropriate choices for configuration parsing and tilde expansion respectively. Version constraints are reasonable.Also applies to: 56-58
keep-cli/contrib/config.toml.example (1)
1-21: LGTM!The example configuration template is well-documented with clear descriptions of each option, valid values, and defaults. All options are commented out by default, which is appropriate for an example file that users can selectively enable.
keep-cli/src/config.rs (6)
7-47: LGTM!The
Argon2ProfileandLogLevelenums are well-designed with appropriate defaults, consistentDisplayimplementations, and case-insensitive deserialization viarename_all = "lowercase".
49-55: LGTM!The path deserializer correctly expands tilde (
~) to the home directory. Not validating path existence at parse time is the right choice—the path may be created later.
73-82: LGTM!Rejecting zero timeout is appropriate—it would cause immediate failures. The validation is sufficient for practical use.
84-109: LGTM!The
Configstruct is well-designed withdeny_unknown_fieldsto catch typos, appropriate field defaults, and a consistent manualDefaultimplementation.
142-158: LGTM!The helper methods
vault_path(),default_relay(), andtimeout_secs()provide sensible fallbacks that are consistent with the documented defaults inconfig.toml.example.
161-245: LGTM! Good test coverage.The tests comprehensively cover parsing, defaults, validation errors, and edge cases. Consider adding a test for
ws://relays (non-TLS) to verify they're accepted alongsidewss://.keep-cli/src/main.rs (5)
436-453: LGTM!The
Servecommand correctly applies the precedence: CLI flags override config defaults. The pattern ofunwrap_or_else(|| cfg.default_relay().to_string())is appropriate.
513-628: LGTM!The
dispatch_frost_networkfunction consistently applies the config default relay across all network commands. TheGroupCreatespecial case forVec<String>is handled correctly.
736-785: LGTM!The
dispatch_confighandler is well-implemented:
Showdisplays effective configuration with defaults appliedPathprovides the config file locationInitsafely checks for existing files before creating, usesinclude_str!for compile-time embedding of the example config
100-105: LGTM!The
ConfigCommandsenum is clean and matches the implementation. The subcommands cover the essential operations for config management.
61-68: Backward compatibility is maintained—relay defaults now come from TOML config.The relay arguments are now
Option<String>, and when not provided, they fall back toConfig::default_relay(), which returns the first configured relay or"wss://nos.lol"if none exist. Users can still pass--relayexplicitly, and those without explicit values benefit from configuration-driven defaults.This is a good change—no breaking changes for existing CLI users, only improved flexibility.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
wksantiago
force-pushed
the
TOML-configuration
branch
from
4319f02 to
8f2fe82
Compare
kwsantiago
force-pushed
the
TOML-configuration
branch
from
8f2fe82 to
b9e667d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@keep-cli/src/config.rs`:
- Around line 114-148: Config::load currently fails if Config::default_path()
returns HomeNotFound, aborting startup even though config is optional; update
Config::load to treat a HomeNotFound from default_path() as "no config" and
return Ok(Self::default()) instead of propagating the error—specifically, call
Self::default_path(), match the Result, and if Err(KeepError::HomeNotFound)
return Ok(Self::default()), otherwise propagate other errors and proceed with
the existing path.exists()/from_file logic; reference functions: Config::load,
Config::default_path, KeepError::HomeNotFound, and Config::default.
Summary
~/.config/keep/config.tomlsupportkeep configsubcommand (show, path, init)Test plan
keep config initto create config filekeep config showto verify settingsSummary by CodeRabbit
New Features
Chores
Style
✏️ Tip: You can customize this high-level summary in your review settings.