various: system notifications #387
Conversation
|
I'd rather if this was done with the notify crate and not zbus. Zbus has a history of holding critical fixes for months, and I'd rather avoid Linux-only features since it turns out as future maintenance work one way or another. Also on a more personal note, this seems to be a bit too much. NixOS icons dependency? zbus? Having to switch between notifications or terminal prompt? Rather drastic imo. We should be able to send a notification using the aforemetioned crate with much less code, including with actions using libnotify. |
|
notify crate makes sense to me, but it'd still depend on zbus, unless you prefer dbus-rs backend (but that would require dependency on libdbus and pkg-config). The terminal prompt was just a fallback in case daemon wasnt available or notification was dismissed/expired, I'm not sure how to handle it in a better way. Also do you think the notification should still use nixos-snowflake icon but just get rid of the dependency or ditch icons all together? |
|
We could inline a SVG for the icon, probably preferable. |
r0chd
force-pushed
the
notifications
branch
3 times, most recently
from
dcd37cd to
b609190
Compare
src/nixos.rs
Outdated
| info!("Apply the config?"); | ||
|
|
||
| let confirmation = match Notification::new() | ||
| .summary(&format!("nh os {variant}")) | ||
| .body("NixOS configuration built successfully.") | ||
| .action("default", "Apply") | ||
| .action("reject", "Reject") | ||
| .show() | ||
| { | ||
| Ok(notification) => { | ||
| let mut confirmation = false; | ||
| notification.wait_for_action(|action| { | ||
| confirmation = match action { | ||
| "default" => true, | ||
| "reject" => false, | ||
| // Notification was closed, fallback to inquire | ||
| _ => inquire::Confirm::new("Apply the config?") | ||
| .with_default(false) | ||
| .prompt() | ||
| .unwrap_or_default(), | ||
| } | ||
| }); | ||
| confirmation | ||
| } | ||
| Err(_) => inquire::Confirm::new("Apply the config?") | ||
| .with_default(false) | ||
| .prompt()?, | ||
| }; |
There was a problem hiding this comment.
I have given this some thought, and I think I prefer the old approach for the prompt. Namely, I think the ask behaviour should remain as-is unless thee user explicitly opts into notification prompts. Otherwise this will be a very invasive change that I, for one, would not appreciate in my setup. Also note that using info!("Apply the config?"); and inquire::Confirm::new("Apply the config?") will actually prompt the user twice. We had previously used info for the question, and the input prompt would not display a message. This has changed with #382.
Imo the correct approach here would be to simply add a --notify flag that coexists with the prompt. Without having checked myself, I think it would be a nice addition to make it so that if --notify flag is available (or maybe something along the lines of --ask=notif, --ask=prompt and --ask=both in addition to --ask that would imply --ask=prompt or similar for backwards compat) the notification pops up with the confirmation prompt alongside the terminal prompt. Though if you think this is too much, then I'd prefer a simple notification for when
- A confirmation prompt is available
- A task is done (e.g., a rebuild)
This can also be used to notify the user of an awaiting sudo prompt if the logic can be made a bit more generic, but that would have to wait until #92 is merged. See here for my previous comment on the matter.
There was a problem hiding this comment.
Flags look good to me, I could wait until #92 as currently notify-rust has a weird behavior with platform specific options where instead of being a noop they're not included in the build hoodie/notify-rust#223 so we're gonna end up with a lot of ugly cfg directives and weird branching, I'm gonna try and merge a PR before doing anything else with this change
There was a problem hiding this comment.
#92 has now been merged.
I'm aiming for a new release at worst on Monday, but if you think that won't be too much of a Rush and you can finish the refactor by then I can try to do it Monday afternoon or on Tuesday morning. Otherwise this'll have to be for the 4.3 release.
I've never used notify-rust before (I was thinking of the other notify crate, for some reason) but waiting for the bug you've linked to be fixed might take too long. If possible, consider alternative libraries.
There was a problem hiding this comment.
I probably won't be able to find time to finish it before the release
WalkthroughAdds desktop notification support and a tri‑state Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant CLI as nh CLI
participant Parse as clap (interface)
participant Notif as notify::NotificationSender
participant Prompt as interactive prompt
Note over CLI,Parse: parse tri-state --ask flag into Option<NotifyAskMode>
U->>CLI: run command (--ask[=prompt|notify|both])
CLI->>Parse: parse ask -> Option<NotifyAskMode>
alt ask is None
CLI->>CLI: proceed without confirmation
else Prompt
CLI->>Prompt: interactive confirm "Confirm the action?"
Prompt-->>CLI: yes/no
CLI->>CLI: continue or bail
else Notify
CLI->>Notif: NotificationSender::new(...).ask()
Notif-->>CLI: true/false
CLI->>CLI: continue or bail
else Both
CLI->>CLI: unimplemented!()
end
sequenceDiagram
autonumber
participant App as Application code
participant Log as logging macro
participant Notif as notify::NotificationSender
participant OS as Desktop Environment
Note over Log,Notif: logging macros send desktop notifications when enabled (NH_NOTIFY=1)
App->>Log: nh_info!("message")
Log->>Notif: NotificationSender::new("nh info", "message").urgency(Normal).send()
Notif->>OS: show notification (if platform permits)
OS-->>Notif: result
Notif-->>Log: Ok / Err (macros unwrap result)
Log->>App: resume
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nixos.rs (1)
1-29: Document MSRV or replaceis_ok_and/is_some_andusages
Norust-versionset in Cargo.toml (and norust-toolchainor workflow pin found) while these methods require Rust ≥ 1.62. Either addrust-version = "1.62"under[package]in Cargo.toml or refactor these calls tomatch/if letfor older Rust support.
♻️ Duplicate comments (1)
src/nixos.rs (1)
280-294: Eliminate panics and fixBothsemantics (don’t auto-confirm).
.ask().unwrap()will panic if notifications fail (e.g., no DBus daemon).NotifyAskMode::Both => truesilently auto-confirms, which contradicts the earlier review guidance to “show notification alongside the prompt,” not bypass it.Apply:
- if let Some(ask) = self.common.ask { - let confirmation = match ask { - NotifyAskMode::Prompt => { - inquire::Confirm::new("Apply the config?") - .with_default(false) - .prompt()? - }, - NotifyAskMode::Notify => { - NotificationSender::new(&format!("nh os {variant}"), "testing") - .ask() - .unwrap() - }, - NotifyAskMode::Both => true, - }; + if let Some(ask) = self.common.ask { + let confirmation = match ask { + NotifyAskMode::Prompt => { + inquire::Confirm::new("Apply the config?") + .with_default(false) + .prompt()? + }, + NotifyAskMode::Notify => { + match NotificationSender::new( + &format!("nh os {variant}"), + &format!("Apply the configuration on {target_hostname}?"), + ).ask() + { + Ok(b) => b, + Err(e) => { + warn!("Notification prompt failed ({e}); falling back to terminal prompt"); + inquire::Confirm::new("Apply the config?") + .with_default(false) + .prompt()? + } + } + }, + // Show a notification but do not block on it; also show the terminal prompt. + // This matches the previously requested behavior. + NotifyAskMode::Both => { + let _ = NotificationSender::new( + &format!("nh os {variant}"), + &format!("Apply the configuration on {target_hostname}?"), + ).send(); + inquire::Confirm::new("Apply the config?") + .with_default(false) + .prompt()? + }, + };Also replace the placeholder body “testing” with a meaningful message.
🧹 Nitpick comments (10)
src/interface.rs (2)
258-263: Document variants and ensure clap help shows choices consistently.Add short docs for Prompt/Notify/Both and mark the args using
value_enumfor consistent help output (like DiffType). Low risk, improves UX.-#[derive(Debug, Clone, ValueEnum, PartialEq)] +#[derive(Debug, Clone, ValueEnum, PartialEq)] pub enum NotifyAskMode { - Prompt, - Notify, - Both, + /// Ask in the terminal (stdin prompt) + Prompt, + /// Ask via a desktop notification action + Notify, + /// Show both notification and terminal prompt (fallback-safe) + Both, }
271-274: Make clap show the accepted values and keep behavior unchanged.
ValueEnumis inferred, but addingvalue_enummirrors other flags and ensures the choices appear in--help. No behavior change.-#[arg(long, short, default_missing_value = "prompt", num_args = 0..=1)] +#[arg(long, short, value_enum, default_missing_value = "prompt", num_args = 0..=1)] pub ask: Option<NotifyAskMode>,Apply to all three: OsRollbackArgs.ask, CommonRebuildArgs.ask, CleanArgs.ask.
Also applies to: 303-306, 427-430
src/commands.rs (2)
441-455: Avoid leaking potentially sensitive CLI args into user-visible notifications.Embedding raw args in the notification summary risks exposing secrets (tokens, URLs, SSH opts) via the desktop’s notification center. Prefer a static summary; keep details in logs.
- if let Err(err) = NotificationSender::new( - &format!( - "nh log {}", - self - .args - .iter() - .map(|s| s.to_string_lossy()) - .collect::<Vec<_>>() - .join(" ") - ), - m, - ) + if let Err(err) = NotificationSender::new("nh log", m) .send() { tracing::debug!(error = ?err, "notification send failed"); }
441-455: Consider honoring dry-run for notifications as well.Currently we notify even when
self.dryis true. If the intent is “no side effects” on dry runs, gate the send behind!self.dry.- if let Some(m) = &self.message { + if let Some(m) = &self.message { + if !self.dry { // notification sending block + } info!("{m}"); }src/home.rs (2)
168-173: Misleading/duplicated --ask warning in Build vs Dry-runWhen variant=Build (not dry), this warns “as dry run was requested.” Also,
run()already warns forhome build. Suggest: warn only for dry-run here; return early for Build without another warn.Apply:
- if self.common.dry || matches!(variant, Build) { - if self.common.ask.is_some() { - warn!("--ask has no effect as dry run was requested"); - } - return Ok(()); - } + if self.common.dry { + if self.common.ask.is_some() { + warn!("--ask has no effect in dry-run mode"); + } + return Ok(()); + } + if matches!(variant, Build) { + return Ok(()); + }
191-196: Unnecessary unsafe around env::set_var
env::set_varis safe. Removeunsafe.Apply:
- unsafe { - env::set_var("HOME_MANAGER_BACKUP_EXT", ext); - } + env::set_var("HOME_MANAGER_BACKUP_EXT", ext);src/darwin.rs (1)
147-150: Ask semantics differ from Linux tri-stateDarwin still treats
askas Option and only prompts in Switch. If/when you bring notification actions to macOS, consider aligning with NotifyAskMode and adding a notification-backed path similar to Linux.Would you like me to prep a follow-up PR to introduce a macOS-specific NotifyAskMode shim that falls back to prompt?
src/notify.rs (2)
19-23: Make notification enablement consistent and configurable (and reuse in ask).
sendrespectsNH_NOTIFY, butaskdoesn’t. Consider centralizing a permissive boolean parser and using it for both paths so users can disable notifications uniformly.Apply:
pub fn send(mut self) -> Result<()> { - let enable_notifications = env::var("NH_NOTIFY").is_ok_and(|v| v == "1"); + let enable_notifications = env::var("NH_NOTIFY") + .ok() + .map(|v| v == "1" || v.eq_ignore_ascii_case("true") || v.eq_ignore_ascii_case("yes")) + .unwrap_or(false); if !enable_notifications { return Ok(()); }Optionally extract this into a small helper and call it from
ask()as an early return (Ok(false)) to allow callers to gracefully fall back to a terminal prompt.
25-28: Avoid overusing Critical urgency; prefer interaction semantics.Setting
Urgency::Criticalfor plain info notifications can be intrusive. If the intent is “make it stick until seen,” consider.timeout(0)(infinite) or a “resident/transient” hint instead of Critical.Example:
#[cfg(all(unix, not(target_os = "macos")))] - self.notification.urgency(Urgency::Critical); + self.notification + .urgency(Urgency::Normal) + .timeout(0); ``` <!-- review_comment_end --> </blockquote></details> <details> <summary>src/nixos.rs (1)</summary><blockquote> `200-215`: **Minor: the “Building …” message is good; consider notifying when activation/elevation is imminent.** A non-blocking info notification right before the first elevated command can improve UX (“About to prompt for sudo”). This aligns with the PR objective. Example (before calling an elevated `Command`): ```rust let _ = NotificationSender::new("nh os", "About to prompt for sudo").send(); ``` <!-- review_comment_end --> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 036c141e2f14fb481f12c4d1498bc5d03d9e1865 and ed84637e355eff2b38c28828e40f16531bfac3b4. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `Cargo.lock` is excluded by `!**/*.lock` </details> <details> <summary>📒 Files selected for processing (10)</summary> * `Cargo.toml` (1 hunks) * `src/clean.rs` (2 hunks) * `src/commands.rs` (2 hunks) * `src/darwin.rs` (2 hunks) * `src/home.rs` (5 hunks) * `src/interface.rs` (3 hunks) * `src/lib.rs` (1 hunks) * `src/main.rs` (1 hunks) * `src/nixos.rs` (6 hunks) * `src/notify.rs` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (3)</summary> <details> <summary>src/clean.rs (1)</summary><blockquote> <details> <summary>src/notify.rs (2)</summary> * `ask` (32-53) * `new` (12-17) </details> </blockquote></details> <details> <summary>src/home.rs (2)</summary><blockquote> <details> <summary>src/nixos.rs (1)</summary> * `fmt` (67-76) </details> <details> <summary>src/notify.rs (2)</summary> * `ask` (32-53) * `new` (12-17) </details> </blockquote></details> <details> <summary>src/nixos.rs (2)</summary><blockquote> <details> <summary>src/home.rs (1)</summary> * `fmt` (52-58) </details> <details> <summary>src/notify.rs (2)</summary> * `ask` (32-53) * `new` (12-17) </details> </blockquote></details> </details> </details> <details> <summary>⏰ 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). (5)</summary> * GitHub Check: Build NH on Linux * GitHub Check: Build NH on Darwin * GitHub Check: treewide-checks * GitHub Check: Test NH on Linux * GitHub Check: Test NH on Darwin </details> <details> <summary>🔇 Additional comments (10)</summary><blockquote> <details> <summary>src/main.rs (1)</summary><blockquote> `13-13`: **LGTM: module wiring only.** The `mod notify;` addition is minimal and aligns with the new module graph. </blockquote></details> <details> <summary>src/lib.rs (1)</summary><blockquote> `14-15`: **LGTM: public surface exposure.** `pub mod notify;` correctly exposes the module for library consumers. </blockquote></details> <details> <summary>src/commands.rs (1)</summary><blockquote> `16-20`: **Import looks correct.** `NotificationSender` import is scoped properly alongside existing items. </blockquote></details> <details> <summary>src/home.rs (2)</summary><blockquote> `10-19`: **Import of NotifyAskMode looks good** Tri-state integration in Home flows makes sense. --- `51-59`: **Nice Display impl for HomeRebuildVariant** Keeps messaging consistent (“build”/“switch”). </blockquote></details> <details> <summary>src/darwin.rs (1)</summary><blockquote> `37-39`: **Build-mode warn message LGTM** Matches behavior that `--ask`/`--dry` don’t affect `darwin build`. </blockquote></details> <details> <summary>src/clean.rs (1)</summary><blockquote> `22-24`: **New imports OK, but see panic path below** Using NotificationSender here is fine; avoid `.unwrap()` downstream. </blockquote></details> <details> <summary>src/nixos.rs (3)</summary><blockquote> `44-47`: **Good guard: `--ask`/`--dry` do nothing for `build`.** Clear UX and avoids misleading flags. <!-- review_comment_end --> --- `66-77`: **Display impl looks correct and is used consistently in messages.** No issues. <!-- review_comment_end --> --- `273-279`: **Early return on dry/build paths is correct; warning is helpful.** No changes needed. <!-- review_comment_end --> </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| humantime = "2.2.0" | ||
| inquire = { default-features = false, version = "0.7.5", features = [ "crossterm" ] } | ||
| nix = { default-features = false, features = [ "fs", "user" ], version = "0.30.1" } | ||
| notify-rust = "4.11.7" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Gate notify-rust per target and pick backend explicitly (avoid accidental zbus coupling).
Given maintainer concerns about zbus and that notifications are Linux-only for now, make the dependency Linux-targeted and select the backend via features. This reduces build surface for darwin/windows and lets us pivot between zbus and dbus cleanly.
Apply one of these diffs:
Option A — keep zbus backend (default-free, explicit):
-[dependencies]
-notify-rust = "4.11.7"
+[dependencies]
+
+[target.'cfg(all(unix, not(target_os = "macos")))'.dependencies]
+notify-rust = { version = "4.11.7", default-features = false, features = ["zbus"] }Option B — switch to dbus-rs backend (requires libdbus/pkg-config in env):
-[dependencies]
-notify-rust = "4.11.7"
+[dependencies]
+
+[target.'cfg(all(unix, not(target_os = "macos")))'.dependencies]
+notify-rust = { version = "4.11.7", default-features = false, features = ["dbus"] }If unsure which features notify-rust exposes in 4.11.7, please confirm:
🌐 Web query:
What backend feature flags does notify-rust 4.11.7 provide (zbus vs dbus), and which are enabled by default?
💡 Result:
Backend feature flags:
- z (uses zbus + serde + async) — this is the default backend. [1][3]
- d (enables dbus, i.e. dbus-rs) — optional; there is also d_vendored to enable dbus/vendored. [1][2]
Which are enabled by default:
- The crate default feature set is ["z"], so the z/zbus backend is enabled by default. To use dbus-rs you must disable default features (or enable d explicitly / set DBUSRS at runtime per the README). [1][2][3]
Sources: notify-rust Cargo.toml and docs (features list). [1] [2] [3]
Gate and select notify-rust backend under Linux target
notify-rust 4.11.7 defaults to ["z"] (zbus + serde + async); use ["d"] for dbus-rs. Move the dependency under a Linux-only target and disable defaults:
Option A — explicit zbus backend:
-[dependencies]
-notify-rust = "4.11.7"
+[target.'cfg(target_os = "linux")'.dependencies]
+notify-rust = { version = "4.11.7", default-features = false, features = ["z"] }Option B — explicit dbus-rs backend:
-[dependencies]
-notify-rust = "4.11.7"
+[target.'cfg(target_os = "linux")'.dependencies]
+notify-rust = { version = "4.11.7", default-features = false, features = ["d"] }🤖 Prompt for AI Agents
In Cargo.toml at line 41, the notify-rust dependency is unconditionally included
and uses default features (zbus), but we must gate it to Linux and select the
dbus-rs backend; move the dependency under a Linux-only target table
([target.'cfg(target_os = "linux")'.dependencies]) and replace the plain entry
with one that disables default-features and sets features = ["d"] (i.e.,
notify-rust = { version = "4.11.7", default-features = false, features = ["d"]
}) so it only builds on Linux and uses the dbus-rs backend.
| if let Some(ask) = args.ask.as_ref() { | ||
| let confirmation = match ask { | ||
| NotifyAskMode::Prompt => { | ||
| Confirm::new("Confirm the cleanup plan?") | ||
| .with_default(false) | ||
| .prompt()? | ||
| }, | ||
| NotifyAskMode::Notify => { | ||
| NotificationSender::new("nh os rollback", "testing") | ||
| .ask() | ||
| .unwrap() | ||
| }, | ||
| NotifyAskMode::Both => true, | ||
| }; | ||
|
|
||
| if !confirmation { | ||
| bail!("User rejected the cleanup plan"); | ||
| } |
There was a problem hiding this comment.
Don’t panic on notification; fix strings; define Both semantics
.ask().unwrap()will panic. Also, notify.rs currently calls.show().unwrap(), so errors can’t be caught—propagate instead.- Strings “nh os rollback”/“testing” look like leftovers; use accurate, user-facing text.
- Treat Both as “notify, else prompt.”
Apply:
- if let Some(ask) = args.ask.as_ref() {
- let confirmation = match ask {
- NotifyAskMode::Prompt => {
- Confirm::new("Confirm the cleanup plan?")
- .with_default(false)
- .prompt()?
- },
- NotifyAskMode::Notify => {
- NotificationSender::new("nh os rollback", "testing")
- .ask()
- .unwrap()
- },
- NotifyAskMode::Both => true,
- };
-
- if !confirmation {
- bail!("User rejected the cleanup plan");
- }
- }
+ if let Some(ask) = args.ask.as_ref() {
+ let confirmation = match ask {
+ NotifyAskMode::Prompt => {
+ Confirm::new("Confirm the cleanup plan?")
+ .with_default(false)
+ .prompt()?
+ },
+ NotifyAskMode::Notify => {
+ match NotificationSender::new("nh clean", "Confirm the cleanup plan?").ask() {
+ Ok(true) => true,
+ Ok(false) => false, // rejected/dismissed
+ Err(e) => {
+ debug!(?e, "Notification failed; falling back to prompt");
+ Confirm::new("Confirm the cleanup plan?")
+ .with_default(false)
+ .prompt()?
+ }
+ }
+ },
+ NotifyAskMode::Both => {
+ match NotificationSender::new("nh clean", "Confirm the cleanup plan?").ask() {
+ Ok(true) => true,
+ _ => {
+ Confirm::new("Confirm the cleanup plan?")
+ .with_default(false)
+ .prompt()?
+ }
+ }
+ },
+ };
+
+ if !confirmation {
+ bail!("User rejected the cleanup plan");
+ }
+ }Follow-up required in notify.rs (not shown here): replace self.notification.show().unwrap() with proper error propagation so callers can fall back:
// in notify.rs ask()
let handle = self.notification.show().wrap_err("Failed to show notification")?;Also consider skipping ask in dry-run for consistency with home/darwin.
🤖 Prompt for AI Agents
In src/clean.rs around lines 310 to 327, the current code unwraps notification
results, uses placeholder strings, and treats Both as always-true; change it to
call ask() and propagate/handle its Result instead of .unwrap(), replace the "nh
os rollback"/"testing" strings with accurate user-facing titles and body (e.g.,
"Home Manager cleanup" and a descriptive message), and implement Both semantics
as "try notify, if notification returns an error or false then fall back to
prompting" so that ask() failures don't panic and the prompt is used as a
fallback; also ensure dry-run paths skip notifications consistently per platform
guidance and rely on the updated notify.rs ask() that returns a Result.
| if let Some(m) = &self.message { | ||
| NotificationSender::new( | ||
| &format!( | ||
| "nh log {}", | ||
| self | ||
| .args | ||
| .iter() | ||
| .map(|s| s.to_string_lossy()) | ||
| .collect::<Vec<_>>() | ||
| .join(" ") | ||
| ), | ||
| m, | ||
| ) | ||
| .send(); | ||
| info!("{m}"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don't ignore Result from NotificationSender::send(); log and continue.
Ignoring a Result from a #[must_use] type triggers unused_must_use and hides real failures. Log at debug and proceed to keep this best-effort.
- NotificationSender::new(
+ // Best-effort: log on failure, don't break execution
+ if let Err(err) = NotificationSender::new(
&format!(
"nh log {}",
self
.args
.iter()
.map(|s| s.to_string_lossy())
.collect::<Vec<_>>()
.join(" ")
),
m,
- )
- .send();
+ )
+ .send()
+ {
+ tracing::debug!(error = ?err, "notification send failed");
+ }📝 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.
| if let Some(m) = &self.message { | |
| NotificationSender::new( | |
| &format!( | |
| "nh log {}", | |
| self | |
| .args | |
| .iter() | |
| .map(|s| s.to_string_lossy()) | |
| .collect::<Vec<_>>() | |
| .join(" ") | |
| ), | |
| m, | |
| ) | |
| .send(); | |
| info!("{m}"); | |
| if let Some(m) = &self.message { | |
| // Best-effort: log on failure, don't break execution | |
| if let Err(err) = NotificationSender::new( | |
| &format!( | |
| "nh log {}", | |
| self | |
| .args | |
| .iter() | |
| .map(|s| s.to_string_lossy()) | |
| .collect::<Vec<_>>() | |
| .join(" ") | |
| ), | |
| m, | |
| ) | |
| .send() | |
| { | |
| tracing::debug!(error = ?err, "notification send failed"); | |
| } | |
| info!("{m}"); | |
| } |
🤖 Prompt for AI Agents
In src/commands.rs around lines 441 to 455, the Result returned by
NotificationSender::send() is currently ignored; capture the Result and handle
the Err case by logging the error at debug (or info) level while continuing
execution (best-effort). Replace the call that discards send()'s return with
code that stores the result and if it is Err(e) calls debug!(...) or
log::debug!(...) with a clear message and the error; do not propagate or
panic—just log and proceed.
| if let Some(ask) = self.common.ask { | ||
| let confirmation = match ask { | ||
| NotifyAskMode::Prompt => { | ||
| inquire::Confirm::new("Apply the config?") | ||
| .with_default(false) | ||
| .prompt()? | ||
| }, | ||
| NotifyAskMode::Notify => true, | ||
| NotifyAskMode::Both => true, | ||
| }; |
There was a problem hiding this comment.
Notify/Both auto-approve is unsafe; use notification actions (with fallback)
Auto-approving defeats the purpose of “ask”. Wire into NotificationSender and only proceed on explicit Accept. Fall back to terminal confirm on notification failure (and prompt on Both).
Apply:
- let confirmation = match ask {
- NotifyAskMode::Prompt => {
- inquire::Confirm::new("Apply the config?")
- .with_default(false)
- .prompt()?
- },
- NotifyAskMode::Notify => true,
- NotifyAskMode::Both => true,
- };
+ let confirmation = match ask {
+ NotifyAskMode::Prompt => {
+ inquire::Confirm::new("Apply the config?")
+ .with_default(false)
+ .prompt()?
+ },
+ NotifyAskMode::Notify => {
+ match crate::notify::NotificationSender::new(
+ &format!("nh home {}", variant),
+ "Apply the config?",
+ ).ask() {
+ Ok(true) => true, // Accepted via notification
+ Ok(false) => false, // Rejected/dismissed
+ Err(e) => {
+ tracing::debug!(?e, "Notification failed; falling back to prompt");
+ inquire::Confirm::new("Apply the config?")
+ .with_default(false)
+ .prompt()?
+ }
+ }
+ },
+ NotifyAskMode::Both => {
+ match crate::notify::NotificationSender::new(
+ &format!("nh home {}", variant),
+ "Apply the config?",
+ ).ask() {
+ Ok(true) => true, // Accepted via notification
+ _ => {
+ inquire::Confirm::new("Apply the config?")
+ .with_default(false)
+ .prompt()?
+ }
+ }
+ },
+ };Additionally (outside the shown range), add the import:
use crate::notify::NotificationSender;Note: On macOS, notify.rs currently returns Ok(false); consider making it an Err to trigger fallback (see clean.rs comment referencing notify.rs).
🤖 Prompt for AI Agents
In src/home.rs around lines 175 to 184, the current logic auto-approves for
Notify and Both which is unsafe; instead, inject/use
crate::notify::NotificationSender, send a notification and only proceed when
NotificationSender returns an explicit Accept (true); if sending the
notification fails, fall back to a terminal confirm prompt (for Notify) and for
Both first attempt notification then fall back to the terminal prompt if
notification fails or returns non-Accept; update the control flow so that Notify
and Both no longer unconditionally return true and ensure you add `use
crate::notify::NotificationSender;` to the imports.
src/notify.rs
Outdated
| pub fn ask(mut self) -> Result<bool> { | ||
| #[cfg(all(unix, not(target_os = "macos")))] | ||
| { | ||
| self.notification.urgency(Urgency::Critical); | ||
| self.notification.action("accept", "Accept"); | ||
| self.notification.action("reject", "Reject"); | ||
| } | ||
|
|
||
| let handle = self.notification.show().unwrap(); | ||
|
|
||
| #[cfg(all(unix, not(target_os = "macos")))] | ||
| { | ||
| let mut confirmation = false; | ||
| handle.wait_for_action(|s| { | ||
| confirmation = s == "accept"; | ||
| }); | ||
| return Ok(confirmation); | ||
| } | ||
|
|
||
| #[cfg(target_os = "macos")] | ||
| Ok(false) | ||
| } |
There was a problem hiding this comment.
Don’t panic on notification delivery; propagate the error and avoid showing on macOS.
show().unwrap() will panic if the server is unavailable (common on minimal/headless systems). Also, the macOS path currently still calls show() before returning Ok(false).
Apply:
pub fn ask(mut self) -> Result<bool> {
- #[cfg(all(unix, not(target_os = "macos")))]
- {
- self.notification.urgency(Urgency::Critical);
- self.notification.action("accept", "Accept");
- self.notification.action("reject", "Reject");
- }
-
- let handle = self.notification.show().unwrap();
-
- #[cfg(all(unix, not(target_os = "macos")))]
- {
- let mut confirmation = false;
- handle.wait_for_action(|s| {
- confirmation = s == "accept";
- });
- return Ok(confirmation);
- }
-
- #[cfg(target_os = "macos")]
- Ok(false)
+ #[cfg(target_os = "macos")]
+ {
+ // Actions aren’t supported via notify-rust on macOS; let callers fall back.
+ return Ok(false);
+ }
+
+ #[cfg(all(unix, not(target_os = "macos")))]
+ {
+ self.notification
+ .urgency(Urgency::Critical)
+ .timeout(0) // require interaction on most servers
+ .action("accept", "Accept")
+ .action("reject", "Reject");
+
+ let handle = self.notification.show()?; // propagate error, don’t panic
+
+ let mut confirmation = false;
+ handle.wait_for_action(|s| {
+ confirmation = s == "accept";
+ });
+ Ok(confirmation)
+ }Optional: set .appname("nh") and consider .icon("computer") or an inlined SVG later per maintainer preference.
📝 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 ask(mut self) -> Result<bool> { | |
| #[cfg(all(unix, not(target_os = "macos")))] | |
| { | |
| self.notification.urgency(Urgency::Critical); | |
| self.notification.action("accept", "Accept"); | |
| self.notification.action("reject", "Reject"); | |
| } | |
| let handle = self.notification.show().unwrap(); | |
| #[cfg(all(unix, not(target_os = "macos")))] | |
| { | |
| let mut confirmation = false; | |
| handle.wait_for_action(|s| { | |
| confirmation = s == "accept"; | |
| }); | |
| return Ok(confirmation); | |
| } | |
| #[cfg(target_os = "macos")] | |
| Ok(false) | |
| } | |
| pub fn ask(mut self) -> Result<bool> { | |
| #[cfg(target_os = "macos")] | |
| { | |
| // Actions aren’t supported via notify-rust on macOS; let callers fall back. | |
| return Ok(false); | |
| } | |
| #[cfg(all(unix, not(target_os = "macos")))] | |
| { | |
| self.notification | |
| .urgency(Urgency::Critical) | |
| .timeout(0) // require interaction on most servers | |
| .action("accept", "Accept") | |
| .action("reject", "Reject"); | |
| let handle = self.notification.show()?; // propagate error, don’t panic | |
| let mut confirmation = false; | |
| handle.wait_for_action(|s| { | |
| confirmation = s == "accept"; | |
| }); | |
| Ok(confirmation) | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/notify.rs around lines 32 to 53, the code currently calls show().unwrap()
(which will panic if the notification server is unavailable) and also invokes
show() even on the macOS branch; change to propagate the notification error
instead of unwrapping and avoid calling show() on macOS. Specifically, make
show() return a Result and use the ? operator (or map_err) to return Err from
ask when notification delivery fails; move the macOS #[cfg(target_os = "macos")]
branch so it returns Ok(false) before any call to show(); keep the unix
action-handling code under the existing unix cfg and use the handle only when
show() succeeded; optionally set .appname("nh") and consider .icon("computer")
on the builder before calling show().
|
Actual yapmachine holy hell |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/home.rs (1)
176-189: Don’t unwrap; fix strings; implement Both with fallback
- Avoid .unwrap() to prevent panics and enable fallback.
- Replace placeholder title/body with accurate, user-facing text.
- Implement Both = “notify, else prompt”.
Apply:
- if let Some(ask) = self.common.ask { - let confirmation = match ask { - NotifyAskMode::Prompt => { - inquire::Confirm::new("Apply the config?") - .with_default(false) - .prompt()? - }, - NotifyAskMode::Notify => { - NotificationSender::new("nh os rollback", "testing") - .ask() - .unwrap() - }, - NotifyAskMode::Both => unimplemented!(), - }; + if let Some(ask) = self.common.ask { + let confirmation = match ask { + NotifyAskMode::Prompt => { + inquire::Confirm::new("Apply the config?") + .with_default(false) + .prompt()? + }, + NotifyAskMode::Notify => { + match NotificationSender::new( + &format!("nh home {variant}"), + "Apply the config?", + ) + .ask() { + Ok(true) => true, + Ok(false) => false, // rejected/dismissed + Err(e) => { + debug!(?e, "Notification failed; falling back to prompt"); + inquire::Confirm::new("Apply the config?") + .with_default(false) + .prompt()? + } + } + }, + NotifyAskMode::Both => { + match NotificationSender::new( + &format!("nh home {variant}"), + "Apply the config?", + ) + .ask() { + Ok(true) => true, + _ => { + inquire::Confirm::new("Apply the config?") + .with_default(false) + .prompt()? + } + } + }, + };src/clean.rs (1)
310-337: No unwraps; fix Both semantics; user-facing text OK
- Replace .ask().unwrap() with proper Result handling and fallback.
- Implement Both = “notify, else prompt”.
- Keep the existing, clear prompt string.
Apply:
- if let Some(ask) = args.ask.as_ref() { + if let Some(ask) = args.ask.as_ref() { let confirmation = match ask { NotifyAskMode::Prompt => { Confirm::new("Confirm the cleanup plan?") .with_default(false) .prompt()? }, NotifyAskMode::Notify => { let clean_mode = match self { Self::Profile(_) => "profile", Self::All(_) => "all", Self::User(_) => "user", }; - - NotificationSender::new( - &format!("nh clean {clean_mode}"), - "Confirm the cleanup plan?", - ) - .ask() - .unwrap() + match NotificationSender::new( + &format!("nh clean {clean_mode}"), + "Confirm the cleanup plan?", + ) + .ask() { + Ok(true) => true, + Ok(false) => false, + Err(e) => { + debug!(?e, "Notification failed; falling back to prompt"); + Confirm::new("Confirm the cleanup plan?") + .with_default(false) + .prompt()? + } + } }, - NotifyAskMode::Both => unimplemented!(), + NotifyAskMode::Both => { + let clean_mode = match self { + Self::Profile(_) => "profile", + Self::All(_) => "all", + Self::User(_) => "user", + }; + match NotificationSender::new( + &format!("nh clean {clean_mode}"), + "Confirm the cleanup plan?", + ) + .ask() { + Ok(true) => true, + _ => { + Confirm::new("Confirm the cleanup plan?") + .with_default(false) + .prompt()? + } + } + }, };src/nixos.rs (1)
474-493: Handle notification errors and fix unimplemented Both mode in rollback path.Same issues as the rebuild confirmation path: using
.unwrap()can panic andBothmode is unimplemented.Apply this fix to handle errors properly and implement the Both mode:
- if let Some(ask) = self.ask.as_ref() { - let confirmation = match ask { - NotifyAskMode::Prompt => { - inquire::Confirm::new(&format!( - "Roll back to generation {}?", - target_generation.number - )) - .with_default(false) - .prompt()? - }, - NotifyAskMode::Notify => { - NotificationSender::new( - "nh os rollback", - &format!("Roll back to generation {}?", target_generation.number), - ) - .ask() - .unwrap() - }, - NotifyAskMode::Both => unimplemented!(), - }; + if let Some(ask) = self.ask.as_ref() { + let confirmation = match ask { + NotifyAskMode::Prompt => { + inquire::Confirm::new(&format!( + "Roll back to generation {}?", + target_generation.number + )) + .with_default(false) + .prompt()? + }, + NotifyAskMode::Notify => { + match NotificationSender::new( + "nh os rollback", + &format!("Roll back to generation {}?", target_generation.number), + ).ask() + { + Ok(b) => b, + Err(e) => { + warn!("Notification prompt failed ({e}); falling back to terminal prompt"); + inquire::Confirm::new(&format!( + "Roll back to generation {}?", + target_generation.number + )) + .with_default(false) + .prompt()? + } + } + }, + NotifyAskMode::Both => { + let _ = NotificationSender::new( + "nh os rollback", + &format!("Roll back to generation {}?", target_generation.number), + ).send(); + inquire::Confirm::new(&format!( + "Roll back to generation {}?", + target_generation.number + )) + .with_default(false) + .prompt()? + }, + };
🧹 Nitpick comments (2)
src/home.rs (1)
196-201: Remove unnecessary unsafe around env::set_varenv::set_var is safe; drop the unsafe block.
- unsafe { - env::set_var("HOME_MANAGER_BACKUP_EXT", ext); - } + env::set_var("HOME_MANAGER_BACKUP_EXT", ext);src/clean.rs (1)
309-337: Skip ask on dry-run for consistency (optional)Consider skipping notifications/prompts when args.dry is set, mirroring home/darwin.
- // Clean the paths - if let Some(ask) = args.ask.as_ref() { + // Clean the paths + if !args.dry { + if let Some(ask) = args.ask.as_ref() { ... - } + } + }
📜 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 (10)
Cargo.toml(1 hunks)src/clean.rs(2 hunks)src/commands.rs(2 hunks)src/darwin.rs(4 hunks)src/home.rs(5 hunks)src/interface.rs(3 hunks)src/lib.rs(1 hunks)src/main.rs(1 hunks)src/nixos.rs(6 hunks)src/notify.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/main.rs
- src/lib.rs
- src/notify.rs
- Cargo.toml
- src/interface.rs
- src/commands.rs
🧰 Additional context used
🧬 Code graph analysis (4)
src/clean.rs (1)
src/notify.rs (2)
ask(32-53)new(12-17)
src/darwin.rs (1)
src/notify.rs (2)
ask(32-53)new(12-17)
src/nixos.rs (3)
src/darwin.rs (1)
fmt(54-60)src/home.rs (1)
fmt(53-59)src/notify.rs (2)
ask(32-53)new(12-17)
src/home.rs (3)
src/darwin.rs (1)
fmt(54-60)src/nixos.rs (1)
fmt(67-76)src/notify.rs (2)
ask(32-53)new(12-17)
⏰ 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). (5)
- GitHub Check: treewide-checks
- GitHub Check: Test NH on Linux
- GitHub Check: Test NH on Darwin
- GitHub Check: Build NH on Linux
- GitHub Check: Build NH on Darwin
🔇 Additional comments (7)
src/home.rs (4)
36-39: Build: correct no-op warning for --ask/--dryMessage and gating look good.
52-60: Display impl LGTMConsistent with darwin/nixos variants.
169-174: Dry-run short-circuit is correctAvoids prompting in dry mode. Good.
176-189: No otherunimplemented!()invocations orNotifyAskMode::Bothusages foundsrc/clean.rs (1)
22-24: Imports LGTMnotify wiring looks consistent.
src/darwin.rs (2)
38-41: Build: correct no-op warning for --ask/--dryGood parity with home.
53-61: Display impl LGTMMatches other variants.
| if let Some(ask) = self.common.ask { | ||
| let confirmation = match ask { | ||
| NotifyAskMode::Prompt => { | ||
| inquire::Confirm::new("Apply the config?") | ||
| .with_default(false) | ||
| .prompt()? | ||
| }, | ||
| NotifyAskMode::Notify => { | ||
| NotificationSender::new("nh os rollback", "testing") | ||
| .ask() | ||
| .unwrap() | ||
| }, | ||
| NotifyAskMode::Both => unimplemented!(), | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Propagate notify errors from notify.rs to enable fallback
ask() currently calls .show().unwrap() (see src/notify.rs). Change it to return Result and let callers fall back.
Additional change outside this file (in src/notify.rs):
- let handle = self.notification.show().unwrap();
+ let handle = self
+ .notification
+ .show()
+ .wrap_err("Failed to show notification")?;Optionally, on macOS return an error to trigger prompt fallback:
- #[cfg(target_os = "macos")]
- Ok(false)
+ #[cfg(target_os = "macos")]
+ Err(eyre!("Notification actions are not supported on macOS"))And provide a send() helper used on macOS:
// in src/notify.rs
pub fn send(self) -> Result<()> {
self.notification.show().map(|_| ()).wrap_err("Failed to show notification")
}🤖 Prompt for AI Agents
In src/home.rs around lines 176-189, the code currently calls
NotificationSender::new(...).ask().unwrap() which panics on notification
failures; change the call to handle and propagate a Result instead (use ? or
match) so callers can fall back to a prompt on error. Update src/notify.rs so
ask()/show() returns Result<()> (do not unwrap internally), add the send()
helper that maps notification.show() into a Result with a wrapped error message,
and ensure platform-specific behavior (on macOS optionally return an Err to
trigger the prompt fallback). Adjust callers in src/home.rs to handle the Result
and fall back to prompting when notification sending fails.
453a5f1 to
bb22a66
Compare
|
@NotAShelf Is this too much voodoo for our purposes, for our mission statement? |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/clean.rs (1)
309-337: Don’t unwrap ask(); implement Both as notify-then-prompt fallbackUnwrap will panic on notification failure; Both is unimplemented. Implement explicit Accept via actions, and fall back to terminal prompt when notification errors or is dismissed.
- if let Some(ask) = args.ask.as_ref() { - let confirmation = match ask { + if let Some(ask) = args.ask.as_ref() { + let confirmation = match ask { NotifyAskMode::Prompt => { Confirm::new("Confirm the cleanup plan?") .with_default(false) .prompt()? }, NotifyAskMode::Notify => { let clean_mode = match self { Self::Profile(_) => "profile", Self::All(_) => "all", Self::User(_) => "user", }; - - NotificationSender::new( - &format!("nh clean {clean_mode}"), - "Confirm the cleanup plan?", - ) - .ask() - .unwrap() + match NotificationSender::new( + &format!("nh clean {clean_mode}"), + "Confirm the cleanup plan?", + ) + .ask() { + Ok(true) => true, // Accepted via notification + Ok(false) => false, // Rejected/dismissed + Err(e) => { + debug!(?e, "Notification failed; falling back to prompt"); + Confirm::new("Confirm the cleanup plan?") + .with_default(false) + .prompt()? + } + } }, - NotifyAskMode::Both => unimplemented!(), + NotifyAskMode::Both => { + let clean_mode = match self { + Self::Profile(_) => "profile", + Self::All(_) => "all", + Self::User(_) => "user", + }; + match NotificationSender::new( + &format!("nh clean {clean_mode}"), + "Confirm the cleanup plan?", + ) + .ask() { + Ok(true) => true, // Accepted via notification + _ => { + Confirm::new("Confirm the cleanup plan?") + .with_default(false) + .prompt()? + } + } + }, }; if !confirmation { bail!("User rejected the cleanup plan"); } }Follow-up in src/notify.rs: replace
.show().unwrap()with proper error propagation so callers can fall back.src/home.rs (1)
176-189: Don’t unwrap ask(); fix strings; implement Both as notify-then-promptAvoid panics, use accurate titles, and fall back to prompt on failure/dismiss.
- if let Some(ask) = self.common.ask { - let confirmation = match ask { + if let Some(ask) = self.common.ask { + let confirmation = match ask { NotifyAskMode::Prompt => { inquire::Confirm::new("Apply the config?") .with_default(false) .prompt()? }, NotifyAskMode::Notify => { - NotificationSender::new("nh os rollback", "testing") - .ask() - .unwrap() + match NotificationSender::new( + &format!("nh home {variant}"), + "Apply the config?", + ).ask() { + Ok(true) => true, + Ok(false) => false, + Err(e) => { + tracing::debug!(?e, "Notification failed; falling back to prompt"); + inquire::Confirm::new("Apply the config?") + .with_default(false) + .prompt()? + } + } }, - NotifyAskMode::Both => unimplemented!(), + NotifyAskMode::Both => { + match NotificationSender::new( + &format!("nh home {variant}"), + "Apply the config?", + ).ask() { + Ok(true) => true, + _ => { + inquire::Confirm::new("Apply the config?") + .with_default(false) + .prompt()? + } + } + }, };src/darwin.rs (1)
158-176: Avoid unwrap; improve prompt text; macOS: notify then promptmacOS lacks actions; send a notification best-effort, then prompt. Don’t panic on failures.
- // MacOS doesn't support notification actions - NotifyAskMode::Notify | NotifyAskMode::Both => { - NotificationSender::new(&format!("nh darwin {variant}"), "testing") - .send() - .unwrap(); - - inquire::Confirm::new("Apply the config?") - .with_default(false) - .prompt()? - }, + // macOS: no actions; notify then prompt + NotifyAskMode::Notify | NotifyAskMode::Both => { + if let Err(e) = NotificationSender::new( + &format!("nh darwin {variant}"), + "Apply the config?", + ).send() { + debug!(?e, "Notification failed; continuing with prompt"); + } + inquire::Confirm::new("Apply the config?") + .with_default(false) + .prompt()? + },
📜 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 (11)
Cargo.toml(1 hunks)src/clean.rs(2 hunks)src/commands.rs(1 hunks)src/darwin.rs(4 hunks)src/home.rs(5 hunks)src/interface.rs(3 hunks)src/lib.rs(1 hunks)src/logging.rs(1 hunks)src/main.rs(1 hunks)src/nixos.rs(7 hunks)src/notify.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- Cargo.toml
- src/nixos.rs
- src/commands.rs
- src/interface.rs
- src/lib.rs
- src/main.rs
- src/notify.rs
🧰 Additional context used
🧬 Code graph analysis (4)
src/logging.rs (1)
src/notify.rs (2)
urgency(41-44)send(60-73)
src/clean.rs (1)
src/notify.rs (2)
ask(94-117)new(25-33)
src/darwin.rs (2)
src/home.rs (1)
fmt(53-59)src/notify.rs (2)
ask(94-117)new(25-33)
src/home.rs (3)
src/darwin.rs (1)
fmt(54-60)src/nixos.rs (1)
fmt(68-77)src/notify.rs (2)
ask(94-117)new(25-33)
🪛 GitHub Actions: Check formating & lints
src/logging.rs
[error] 133-133: cargo fmt --check detected formatting differences in this file. Run 'cargo fmt' to fix code style issues.
⏰ 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). (4)
- GitHub Check: Build NH on Darwin
- GitHub Check: Test NH on Linux
- GitHub Check: Build NH on Linux
- GitHub Check: Test NH on Darwin
🔇 Additional comments (5)
src/logging.rs (1)
92-135: Fix formatting to satisfy cargo fmtLong single-line expressions likely triggered the formatter failure reported by CI. The above refactor also reflows lines; still, please run cargo fmt locally.
src/home.rs (2)
36-39: Good guard for build mode/dry-runWarning users that --ask/--dry have no effect for build is appropriate.
52-60: Display impl LGTMClear, minimal Display for variant; matches nixos/darwin patterns elsewhere.
src/darwin.rs (2)
38-42: Good UX warningsClear message that --ask/--dry don’t affect build mode.
53-61: Display impl LGTMMatches Home/NixOS variants; concise.
| #[macro_export] | ||
| macro_rules! nh_trace { | ||
| ($($arg:tt)*) => { | ||
| use notify_rust::Urgency; | ||
| use crate::notify::NotificationSender; | ||
| let message = format!($($arg)*); | ||
| tracing::trace!($($arg)*); | ||
| NotificationSender::new("nh trace", &message).urgency(Urgency::Low).send().unwrap(); | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t panic in logging macros; use $crate path and handle send() errors
Unwrapping here can crash on D-Bus/libnotify failures. Also prefer $crate::... in macros so downstream crates can use them without path breakage.
#[macro_export]
macro_rules! nh_trace {
($($arg:tt)*) => {
- use notify_rust::Urgency;
- use crate::notify::NotificationSender;
- let message = format!($($arg)*);
- tracing::trace!($($arg)*);
- NotificationSender::new("nh trace", &message).urgency(Urgency::Low).send().unwrap();
+ let message = format!($($arg)*);
+ tracing::trace!($($arg)*);
+ if let Err(e) = $crate::notify::NotificationSender::new("nh trace", &message)
+ .urgency(::notify_rust::Urgency::Low)
+ .send()
+ {
+ tracing::debug!(?e, "Failed to send nh trace notification");
+ }
};
}
#[macro_export]
macro_rules! nh_debug {
($($arg:tt)*) => {
- use notify_rust::Urgency;
- use crate::notify::NotificationSender;
-
- let message = format!($($arg)*);
- tracing::debug!($($arg)*);
- NotificationSender::new("nh debug", &message).urgency(Urgency::Low).send().unwrap();
+ let message = format!($($arg)*);
+ tracing::debug!($($arg)*);
+ if let Err(e) = $crate::notify::NotificationSender::new("nh debug", &message)
+ .urgency(::notify_rust::Urgency::Low)
+ .send()
+ {
+ tracing::debug!(?e, "Failed to send nh debug notification");
+ }
};
}
#[macro_export]
macro_rules! nh_info {
($($arg:tt)*) => {
- use notify_rust::Urgency;
- use crate::notify::NotificationSender;
- let message = format!($($arg)*);
- tracing::info!($($arg)*);
- NotificationSender::new("nh info", &message).urgency(Urgency::Normal).send().unwrap();
+ let message = format!($($arg)*);
+ tracing::info!($($arg)*);
+ if let Err(e) = $crate::notify::NotificationSender::new("nh info", &message)
+ .urgency(::notify_rust::Urgency::Normal)
+ .send()
+ {
+ tracing::debug!(?e, "Failed to send nh info notification");
+ }
};
}
#[macro_export]
macro_rules! nh_warn {
($($arg:tt)*) => {
- use notify_rust::Urgency;
- use crate::notify::NotificationSender;
- let message = format!($($arg)*);
- tracing::warn!($($arg)*);
- NotificationSender::new("nh warn", &message).urgency(Urgency::Normal).send().unwrap();
+ let message = format!($($arg)*);
+ tracing::warn!($($arg)*);
+ if let Err(e) = $crate::notify::NotificationSender::new("nh warn", &message)
+ .urgency(::notify_rust::Urgency::Normal)
+ .send()
+ {
+ tracing::debug!(?e, "Failed to send nh warn notification");
+ }
};
}Also applies to: 103-113, 115-124, 126-135
🤖 Prompt for AI Agents
In src/logging.rs around lines 92 to 101 (and similarly for 103-113, 115-124,
126-135), the macros currently use bare paths and unwrap() on
NotificationSender::send(), which can panic and break downstream crates; change
macro references to use $crate::notify::NotificationSender and
$crate::notify::NotificationSender::new (and fully qualified
notify_rust::Urgency via ::notify_rust::Urgency if needed) so the macros resolve
correctly for downstream users, and replace the .send().unwrap() with
non-panicking error handling (e.g. capture the Result and, on Err(e), emit a
tracing::error! or tracing::debug! with the error) so failures to send
notifications do not cause a panic.
| #[macro_export] | ||
| macro_rules! nh_trace { | ||
| ($($arg:tt)*) => { | ||
| use notify_rust::Urgency; | ||
| use crate::notify::NotificationSender; | ||
| let message = format!($($arg)*); | ||
| tracing::trace!($($arg)*); | ||
| NotificationSender::new("nh trace", &message).urgency(Urgency::Low).send().unwrap(); | ||
| }; | ||
| } | ||
|
|
||
| #[macro_export] | ||
| macro_rules! nh_debug { | ||
| ($($arg:tt)*) => { | ||
| use notify_rust::Urgency; | ||
| use crate::notify::NotificationSender; | ||
|
|
||
| let message = format!($($arg)*); | ||
| tracing::debug!($($arg)*); | ||
| NotificationSender::new("nh debug", &message).urgency(Urgency::Low).send().unwrap(); | ||
| }; | ||
| } | ||
|
|
||
| #[macro_export] | ||
| macro_rules! nh_info { | ||
| ($($arg:tt)*) => { | ||
| use notify_rust::Urgency; | ||
| use crate::notify::NotificationSender; | ||
| let message = format!($($arg)*); | ||
| tracing::info!($($arg)*); | ||
| NotificationSender::new("nh info", &message).urgency(Urgency::Normal).send().unwrap(); | ||
| }; | ||
| } | ||
|
|
||
| #[macro_export] | ||
| macro_rules! nh_warn { | ||
| ($($arg:tt)*) => { | ||
| use notify_rust::Urgency; | ||
| use crate::notify::NotificationSender; | ||
| let message = format!($($arg)*); | ||
| tracing::warn!($($arg)*); | ||
| NotificationSender::new("nh warn", &message).urgency(Urgency::Normal).send().unwrap(); | ||
| }; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Macro args: format!($($arg)*) will fail on structured tracing fields
These macros will not compile for calls like nh_info!(%val, key=?x) because format! requires a format string literal. Either constrain usage to literal message forms or add macro arms to handle “message-first” vs. “structured fields” cases, and fall back to a static notification message for non-literal forms.
Run to find non-literal uses:
🏁 Script executed:
#!/bin/bash
# Lists nh_* macro invocations that DO NOT start with a string literal
rg -nP --type=rust 'nh_(trace|debug|info|warn)!\(\s*"(?:[^"\\]|\\.)*"' -v src | sed -n '1,200p'Length of output: 8911
🏁 Script executed:
#!/bin/bash
# Search for any uses of nh_trace!, nh_debug!, nh_info!, or nh_warn! macros
rg -n --type=rust 'nh_(trace|debug|info|warn)!\(' srcLength of output: 122
🏁 Script executed:
#!/bin/bash
# Find all uses of nh_* macros outside their definition file
rg -nP --type=rust 'nh_(?:trace|debug|info|warn)!\s*\(' -g '!src/logging.rs' srcLength of output: 0
Support structured fields or restrict macros to literal-only messages
The nh_* macros call format!($($arg)*), so invocations like nh_info!(%val, key=?x) won’t compile.
- Either constrain these macros to literal‐only messages (and document/enforce this restriction)
- Or add additional arms to capture a format string plus structured fields, falling back to a safe notification body when fields are used
src/logging.rs:92–135
🧰 Tools
🪛 GitHub Actions: Check formating & lints
[error] 133-133: cargo fmt --check detected formatting differences in this file. Run 'cargo fmt' to fix code style issues.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/notify.rs (1)
104-117: Propagate errors and avoid calling show() on macOS; add actions/timeout under UnixUnwrap will panic; also macOS branch still calls show(). Make ask() return Err on macOS to trigger fallback, and use actions + timeout with proper error propagation.
-use color_eyre::eyre::Result; +use color_eyre::eyre::{Result, eyre}; @@ - pub fn ask(mut self) -> Result<bool> { - #[cfg(all(unix, not(target_os = "macos")))] - { - self - .notification - .urgency(self.urgency.unwrap_or(Urgency::Critical)); - self.notification.action("accept", "Accept"); - self.notification.action("reject", "Reject"); - } - - let handle = self.notification.show().unwrap(); - - #[cfg(all(unix, not(target_os = "macos")))] - { - let mut confirmation = false; - handle.wait_for_action(|s| { - confirmation = s == "accept"; - }); - Ok(confirmation) - } - - #[cfg(target_os = "macos")] - Ok(false) - } + pub fn ask(mut self) -> Result<bool> { + #[cfg(target_os = "macos")] + { + // Trigger caller fallback (prompt) on unsupported platform. + return Err(eyre!("Notification actions are not supported on macOS")); + } + #[cfg(all(unix, not(target_os = "macos")))] + { + self + .notification + .urgency(self.urgency.unwrap_or(Urgency::Critical)) + .timeout(0) // require interaction on most servers + .action("accept", "Accept") + .action("reject", "Reject"); + let handle = self.notification.show()?; // propagate error + let mut confirmation = false; + handle.wait_for_action(|s| { + confirmation = s == "accept"; + }); + Ok(confirmation) + } + }src/clean.rs (2)
309-332: Skip ask in dry-run for consistency with home/darwinGuard the entire ask block with if !args.dry to avoid prompting when nothing will be executed.
309-332: Don’t unwrap ask(); implement Both semantics and fallback to promptUnwrap will panic if the server is unavailable; Both is unimplemented. Use explicit Accept, otherwise prompt fallback; log and continue on errors.
if let Some(ask) = args.ask.as_ref() { let confirmation = match ask { NotifyAskMode::Prompt => { Confirm::new("Confirm the cleanup plan?") .with_default(false) .prompt()? }, NotifyAskMode::Notify => { let clean_mode = match self { Self::Profile(_) => "profile", Self::All(_) => "all", Self::User(_) => "user", }; - - NotificationSender::new( - &format!("nh clean {clean_mode}"), - "Confirm the cleanup plan?", - ) - .ask() - .unwrap() + match NotificationSender::new( + &format!("nh clean {clean_mode}"), + "Confirm the cleanup plan?", + ).ask() { + Ok(true) => true, // accepted via notification + Ok(false) => false, // rejected/dismissed + Err(e) => { + debug!(?e, "Notification failed; falling back to prompt"); + Confirm::new("Confirm the cleanup plan?") + .with_default(false) + .prompt()? + } + } }, - NotifyAskMode::Both => unimplemented!(), + NotifyAskMode::Both => { + let clean_mode = match self { + Self::Profile(_) => "profile", + Self::All(_) => "all", + Self::User(_) => "user", + }; + match NotificationSender::new( + &format!("nh clean {clean_mode}"), + "Confirm the cleanup plan?", + ).ask() { + Ok(true) => true, // accepted via notification + _ => { + Confirm::new("Confirm the cleanup plan?") + .with_default(false) + .prompt()? + } + } + }, };src/home.rs (1)
176-189: Fix placeholder strings, remove unwrap, and implement Both with fallbackUse a meaningful summary, avoid panic on notification failure, and wire Both as “try notify, else prompt.”
if let Some(ask) = self.common.ask { let confirmation = match ask { NotifyAskMode::Prompt => { inquire::Confirm::new("Apply the config?") .with_default(false) .prompt()? }, NotifyAskMode::Notify => { - NotificationSender::new("nh os rollback", "testing") - .ask() - .unwrap() + match NotificationSender::new( + &format!("nh home {}", variant), + "Apply the config?", + ).ask() { + Ok(true) => true, + Ok(false) => false, + Err(e) => { + tracing::debug!(?e, "Notification failed; falling back to prompt"); + inquire::Confirm::new("Apply the config?") + .with_default(false) + .prompt()? + } + } }, - NotifyAskMode::Both => unimplemented!(), + NotifyAskMode::Both => { + match NotificationSender::new( + &format!("nh home {}", variant), + "Apply the config?", + ).ask() { + Ok(true) => true, + _ => { + inquire::Confirm::new("Apply the config?") + .with_default(false) + .prompt()? + } + } + }, };
🧹 Nitpick comments (2)
src/notify.rs (2)
85-90: Doc tweak to reflect macOS fallback via errorThe docs say Ok(false) on unsupported platforms; with the change above we return Err to enable prompt fallback.
- /// On unsupported platforms (e.g., macOS, Windows), this function always - /// returns `Ok(false)` since interactive actions are not supported. + /// On unsupported platforms (e.g., macOS), this function returns an error so + /// callers can fall back to a terminal prompt.
25-33: Set appname to “nh” for better source attribution in notification centersMinor polish; helps users identify the app quickly.
pub fn new(summary: &str, body: &str) -> Self { let mut notification = Notification::new(); + notification.appname("nh"); notification.summary(summary); notification.body(body);
📜 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 (11)
Cargo.toml(1 hunks)src/clean.rs(2 hunks)src/commands.rs(1 hunks)src/darwin.rs(4 hunks)src/home.rs(5 hunks)src/interface.rs(3 hunks)src/lib.rs(1 hunks)src/logging.rs(1 hunks)src/main.rs(1 hunks)src/nixos.rs(7 hunks)src/notify.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/main.rs
- Cargo.toml
- src/lib.rs
- src/logging.rs
- src/commands.rs
- src/nixos.rs
- src/darwin.rs
- src/interface.rs
🧰 Additional context used
🧬 Code graph analysis (2)
src/home.rs (3)
src/darwin.rs (1)
fmt(54-60)src/nixos.rs (1)
fmt(68-77)src/notify.rs (2)
ask(94-117)new(25-33)
src/clean.rs (1)
src/notify.rs (2)
ask(94-117)new(25-33)
⏰ 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). (4)
- GitHub Check: Test NH on Darwin
- GitHub Check: Test NH on Linux
- GitHub Check: Build NH on Linux
- GitHub Check: Build NH on Darwin
🔇 Additional comments (2)
src/home.rs (2)
52-60: LGTM: Display impl for HomeRebuildVariantSimple and correct stringification.
169-174: LGTM: Early return on dry/build with warningMatches the CLI UX you described; avoids prompting when no activation occurs.
r0chd
force-pushed
the
notifications
branch
from
7c65e7b to
a27af9b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nixos.rs (1)
322-355: Switch variant calls wrong actions (test/boot) and runs two activations.For
Switchyou callswitch-to-configuration testand laterboot. This is incorrect;Switchshould callswitchonce. Refactor to a single match overvariantthat passes the right subcommand.Apply:
- if let Test | Switch = variant { - let switch_to_configuration = - target_profile.join("bin").join("switch-to-configuration"); - ... - Command::new(switch_to_configuration) - .arg("test") - .ssh(self.target_host.clone()) - .message("Activating configuration") - .elevate(elevate.then_some(elevation.clone())) - .preserve_envs(["NIXOS_INSTALL_BOOTLOADER"]) - .with_required_env() - .run() - .wrap_err("Activation (test) failed")?; - } - - if let Boot | Switch = variant { - let canonical_out_path = out_path - .canonicalize() - .context("Failed to resolve output path")?; - ... - Command::new(switch_to_configuration) - .arg("boot") - .ssh(self.target_host) - .elevate(elevate.then_some(elevation)) - .message("Adding configuration to bootloader") - .preserve_envs(["NIXOS_INSTALL_BOOTLOADER"]) - .with_required_env() - .run() - .wrap_err("Bootloader activation failed")?; - } + match variant { + Test => { + let switch_to_configuration = + target_profile.join("bin").join("switch-to-configuration"); + if !switch_to_configuration.exists() { + return Err(eyre!(/* unchanged help text */)); + } + let switch_to_configuration = + switch_to_configuration.canonicalize().context("Failed to resolve switch-to-configuration path")?; + let switch_to_configuration = + switch_to_configuration.to_str().ok_or_else(|| eyre!("switch-to-configuration path contains invalid UTF-8"))?; + Command::new(switch_to_configuration) + .arg("test") + .ssh(self.target_host.clone()) + .message("Activating configuration") + .elevate(elevate.then_some(elevation.clone())) + .preserve_envs(["NIXOS_INSTALL_BOOTLOADER"]) + .with_required_env() + .run() + .wrap_err("Activation (test) failed")?; + } + Switch => { + let canonical_out_path = out_path.canonicalize().context("Failed to resolve output path")?; + Command::new("nix") + .elevate(elevate.then_some(elevation.clone())) + .args(["build", "--no-link", "--profile", SYSTEM_PROFILE]) + .arg(&canonical_out_path) + .ssh(self.target_host.clone()) + .with_required_env() + .run() + .wrap_err("Failed to set system profile")?; + + let switch_to_configuration = + out_path.join("bin").join("switch-to-configuration"); + if !switch_to_configuration.exists() { + return Err(eyre!(/* unchanged help text */)); + } + let switch_to_configuration = + switch_to_configuration.canonicalize().context("Failed to resolve switch-to-configuration path")?; + let switch_to_configuration = + switch_to_configuration.to_str().ok_or_else(|| eyre!("switch-to-configuration path contains invalid UTF-8"))?; + Command::new(switch_to_configuration) + .arg("switch") + .ssh(self.target_host.clone()) + .elevate(elevate.then_some(elevation.clone())) + .message("Activating configuration") + .preserve_envs(["NIXOS_INSTALL_BOOTLOADER"]) + .with_required_env() + .run() + .wrap_err("Activation (switch) failed")?; + } + Boot => { + let canonical_out_path = out_path.canonicalize().context("Failed to resolve output path")?; + Command::new("nix") + .elevate(elevate.then_some(elevation.clone())) + .args(["build", "--no-link", "--profile", SYSTEM_PROFILE]) + .arg(&canonical_out_path) + .ssh(self.target_host.clone()) + .with_required_env() + .run() + .wrap_err("Failed to set system profile")?; + + let switch_to_configuration = + out_path.join("bin").join("switch-to-configuration"); + if !switch_to_configuration.exists() { + return Err(eyre!(/* unchanged help text */)); + } + let switch_to_configuration = + switch_to_configuration.canonicalize().context("Failed to resolve switch-to-configuration path")?; + let switch_to_configuration = + switch_to_configuration.to_str().ok_or_else(|| eyre!("switch-to-configuration path contains invalid UTF-8"))?; + Command::new(switch_to_configuration) + .arg("boot") + .ssh(self.target_host.clone()) + .elevate(elevate.then_some(elevation.clone())) + .message("Adding configuration to bootloader") + .preserve_envs(["NIXOS_INSTALL_BOOTLOADER"]) + .with_required_env() + .run() + .wrap_err("Bootloader activation failed")?; + } + Build | BuildVm => { /* already returned earlier */ } + }Also applies to: 357-403
♻️ Duplicate comments (3)
src/notify.rs (1)
94-117: Don’t unwrap; avoid calling show() on macOS; gate platform paths cleanly.
show().unwrap()will panic if no server is available; alsoshow()is called unconditionally before the macOSOk(false)branch. Moveshow()into the Unix branch and propagate errors. Add a no‑Unix fallback returningOk(false).Apply:
pub fn ask(mut self) -> Result<bool> { - #[cfg(all(unix, not(target_os = "macos")))] - { - self - .notification - .urgency(self.urgency.unwrap_or(Urgency::Critical)); - self.notification.action("accept", "Accept"); - self.notification.action("reject", "Reject"); - } - - let handle = self.notification.show().unwrap(); - - #[cfg(all(unix, not(target_os = "macos")))] - { - let mut confirmation = false; - handle.wait_for_action(|s| { - confirmation = s == "accept"; - }); - Ok(confirmation) - } - - #[cfg(target_os = "macos")] - Ok(false) + #[cfg(target_os = "macos")] + { + // Actions unsupported via notify-rust on macOS; let callers fall back. + return Ok(false); + } + + #[cfg(all(unix, not(target_os = "macos")))] + { + self.notification + .urgency(self.urgency.unwrap_or(Urgency::Critical)) + .timeout(0) + .action("accept", "Accept") + .action("reject", "Reject"); + + let handle = self.notification.show()?; // propagate error + let mut confirmation = false; + handle.wait_for_action(|s| { + confirmation = s == "accept"; + }); + return Ok(confirmation); + } + + #[cfg(not(unix))] + { + // Unsupported platform: no interactive actions. + Ok(false) + } }src/nixos.rs (2)
475-494: Mirror the ask() fix in rollback; remove unwrap and implement Both.Same unwrap panic and unimplemented
Bothas rebuild. Fall back to prompt on error; send+prompt forBoth.Apply:
- if let Some(ask) = self.ask.as_ref() { - let confirmation = match ask { - NotifyAskMode::Prompt => { - inquire::Confirm::new(&format!( - "Roll back to generation {}?", - target_generation.number - )) - .with_default(false) - .prompt()? - }, - NotifyAskMode::Notify => { - NotificationSender::new( - "nh os rollback", - &format!("Roll back to generation {}?", target_generation.number), - ) - .ask() - .unwrap() - }, - NotifyAskMode::Both => unimplemented!(), - }; + if let Some(ask) = self.ask.as_ref() { + let question = format!("Roll back to generation {}?", target_generation.number); + let confirmation = match ask { + NotifyAskMode::Prompt => { + inquire::Confirm::new(&question).with_default(false).prompt()? + }, + NotifyAskMode::Notify => { + match NotificationSender::new("nh os rollback", &question).ask() { + Ok(b) => b, + Err(e) => { + warn!("Notification prompt failed ({e}); falling back to terminal prompt"); + inquire::Confirm::new(&question).with_default(false).prompt()? + } + } + }, + NotifyAskMode::Both => { + let _ = NotificationSender::new("nh os rollback", &question).send(); + inquire::Confirm::new(&question).with_default(false).prompt()? + }, + };
281-297: Avoid panic on notification ask; implement Both semantics with prompt fallback.
.unwrap()can panic andBothisunimplemented!(). Propagate errors and show a terminal prompt on failure; forBoth, send a passive notification and still prompt.Apply:
- if let Some(ask) = self.common.ask { - let confirmation = match ask { - NotifyAskMode::Prompt => { - inquire::Confirm::new("Apply the config?") - .with_default(false) - .prompt()? - }, - NotifyAskMode::Notify => { - NotificationSender::new( - &format!("nh os {variant}"), - "Apply the config?", - ) - .ask() - .unwrap() - }, - NotifyAskMode::Both => unimplemented!(), - }; + if let Some(ask) = self.common.ask { + let question = "Apply the config?"; + let confirmation = match ask { + NotifyAskMode::Prompt => { + inquire::Confirm::new(question).with_default(false).prompt()? + }, + NotifyAskMode::Notify => { + match NotificationSender::new(&format!("nh os {variant}"), question).ask() { + Ok(b) => b, + Err(e) => { + warn!("Notification prompt failed ({e}); falling back to terminal prompt"); + inquire::Confirm::new(question).with_default(false).prompt()? + } + } + }, + NotifyAskMode::Both => { + let _ = NotificationSender::new(&format!("nh os {variant}"), question).send(); + inquire::Confirm::new(question).with_default(false).prompt()? + }, + };
🧹 Nitpick comments (4)
src/notify.rs (4)
85-90: Doc/platform mismatch (Windows).Comment says “macOS, Windows” return
Ok(false), but only macOS is handled. Either implement a non‑Unix fallback (see previous diff) or narrow the doc to macOS.
25-33: Set app name for notification grouping.Add
.appname("nh")once at construction to improve DE integration.pub fn new(summary: &str, body: &str) -> Self { let mut notification = Notification::new(); + notification.appname("nh"); notification.summary(summary); notification.body(body);
60-73: Optional: carry urgency across platforms and consider a sane default timeout.If acceptable, set a short
.timeout(...)for non‑interactive sends and optionally an icon. Keep the currentNH_NOTIFYgate.Would you like me to wire a cross‑platform icon (or inline SVG) per maintainer preference?
1-11: Minor: use the imported type in the field.
notification: Notificationreads cleaner thannotify_rust::NotificationsinceNotificationis imported.
📜 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 (11)
Cargo.toml(1 hunks)src/clean.rs(2 hunks)src/commands.rs(1 hunks)src/darwin.rs(4 hunks)src/home.rs(5 hunks)src/interface.rs(3 hunks)src/lib.rs(1 hunks)src/logging.rs(1 hunks)src/main.rs(1 hunks)src/nixos.rs(7 hunks)src/notify.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- src/lib.rs
- Cargo.toml
- src/commands.rs
- src/main.rs
- src/logging.rs
- src/home.rs
- src/clean.rs
- src/darwin.rs
- src/interface.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/nixos.rs (3)
src/darwin.rs (1)
fmt(54-60)src/home.rs (1)
fmt(53-59)src/notify.rs (2)
ask(94-117)new(25-33)
⏰ 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). (4)
- GitHub Check: Build NH on Darwin
- GitHub Check: Build NH on Linux
- GitHub Check: Test NH on Linux
- GitHub Check: Test NH on Darwin
🔇 Additional comments (3)
src/notify.rs (1)
60-60: Should ask() observe NH_NOTIFY?
send()is gated byNH_NOTIFY=="1",ask()is not. If the env var is intended as a global kill‑switch, mirror the gate inask(); if not, document thatask()ignoresNH_NOTIFY.src/nixos.rs (2)
67-78: Display impl reads well.Variant strings are consistent and used across messages. LGTM.
45-47: Good UX: warn on ineffective flags.Clear message when
--ask/--drydon’t apply tobuild. LGTM.
| if let Some(ask) = self.common.ask { | ||
| let confirmation = match ask { | ||
| NotifyAskMode::Prompt => { | ||
| inquire::Confirm::new("Apply the config?") | ||
| .with_default(false) | ||
| .prompt()? | ||
| }, | ||
| NotifyAskMode::Notify => { | ||
| NotificationSender::new( | ||
| &format!("nh os {variant}"), | ||
| "Apply the config?", | ||
| ) | ||
| .ask() | ||
| .unwrap() | ||
| }, | ||
| NotifyAskMode::Both => unimplemented!(), | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Repo‑wide check for unwraps and unimplemented Both.
Search for other NotificationSender::ask().unwrap() and NotifyAskMode::Both => unimplemented!() occurrences.
Also applies to: 475-494
🏁 Script executed:
#!/bin/bash
# Find unsafe unwraps on interactive notifications
rg -nP -C2 'NotificationSender::new\([^)]*\)\.ask\(\)\.unwrap\(\)'
# Find unimplemented Both branches
rg -nP -C2 'NotifyAskMode::Both\s*=>\s*unimplemented!\s*\('Length of output: 705
Replace NotificationSender::ask().unwrap() with proper error handling and implement NotifyAskMode::Both
Do not unwrap the Result from ask(); propagate or handle errors and implement a concrete behavior for NotifyAskMode::Both (e.g., prompt then notify or a defined non-blocking flow).
- src/home.rs: lines 186–189
- src/nixos.rs: lines 294–297
- src/nixos.rs: lines 491–494
- src/clean.rs: lines 329–332
🤖 Prompt for AI Agents
In src/nixos.rs around lines 281-297, the code currently calls
NotificationSender::ask().unwrap() and leaves NotifyAskMode::Both unimplemented;
replace the unwrap by properly handling or propagating the Result (use ? to
return the error or match the Result to log/convert the error into the
function's error type) and implement NotifyAskMode::Both with a concrete flow
(for example: prompt the user first and if they confirm, send a notification; or
perform a non-blocking notify and then prompt — pick one consistent behavior).
Ensure the function signature supports returning a Result so you can propagate
errors from ask(), update callers if necessary, and mirror this same
error-handling and Both-implementation approach in the other referenced
locations (src/home.rs lines 186–189, src/nixos.rs lines 491–494, src/clean.rs
lines 329–332).
a27af9b to
125c048
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/nixos.rs (2)
281-296:NotifyAskMode::Bothpanics at runtime (unimplemented!)Calling
--ask=bothwill abort. Implement non-invasive “Both” semantics: show a non-blocking notification and still prompt in the terminal.Apply:
- NotifyAskMode::Both => unimplemented!(), + NotifyAskMode::Both => { + let _ = NotificationSender::new( + &format!("nh os {variant}"), + "Apply the config?", + ).send(); + inquire::Confirm::new("Apply the config?") + .with_default(false) + .prompt()? + },
474-492:NotifyAskMode::Bothalso unimplemented in rollbackSame panic risk as rebuild. Mirror the “Both = notify (non-blocking) + prompt” pattern and provide a specific body.
Apply:
- NotifyAskMode::Both => unimplemented!(), + NotifyAskMode::Both => { + let _ = NotificationSender::new( + "nh os rollback", + &format!("Roll back to generation {}?", target_generation.number), + ).send(); + inquire::Confirm::new(&format!( + "Roll back to generation {}?", + target_generation.number + )) + .with_default(false) + .prompt()? + },
🧹 Nitpick comments (1)
src/nixos.rs (1)
274-279: Misleading warn message when not a dry runThis branch triggers for Build/BuildVm or dry-run; the message blames only dry-run.
Apply:
- if self.common.ask.is_some() { - warn!("--ask has no effect as dry run was requested"); - } + if self.common.ask.is_some() { + warn!("--ask has no effect for `nh os {variant}` or when using --dry"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/clean.rs(2 hunks)src/commands.rs(3 hunks)src/darwin.rs(4 hunks)src/generations.rs(2 hunks)src/home.rs(5 hunks)src/interface.rs(3 hunks)src/logging.rs(1 hunks)src/nixos.rs(7 hunks)src/notify.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/commands.rs
- src/home.rs
- src/notify.rs
- src/logging.rs
- src/darwin.rs
- src/interface.rs
- src/clean.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/nixos.rs (3)
src/darwin.rs (1)
fmt(54-60)src/home.rs (1)
fmt(53-59)src/notify.rs (2)
ask(95-118)new(25-33)
⏰ 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). (5)
- GitHub Check: Build NH on Linux
- GitHub Check: Build NH on Darwin
- GitHub Check: Test NH on Linux
- GitHub Check: Test NH on Darwin
- GitHub Check: treewide-checks
🔇 Additional comments (4)
src/nixos.rs (3)
67-78: Nice: Display impl for OsRebuildVariantClear, human-friendly variant names; useful for logs/UI.
45-47: Good guardrail forbuild: warns early and avoids noop flagsThis matches expected UX for
nh os build.
404-405: LGTM: final path debug messageHelpful completion trace with
nh_debug!.src/generations.rs (1)
342-347: Formatting bug: width specifier is invalid; will not compile
{name:<width$}uses a non-numeric positional specifier. Use dynamic width via nested braces or passwidthas a named arg.Apply:
- format!("{name:<width$}") + format!("{name:<{width}}", width = width)Likely an incorrect or invalid review comment.
| if let Some(ask) = self.common.ask { | ||
| let confirmation = match ask { | ||
| NotifyAskMode::Prompt => { | ||
| inquire::Confirm::new("Apply the config?") | ||
| .with_default(false) | ||
| .prompt()? | ||
| }, | ||
| NotifyAskMode::Notify => { | ||
| NotificationSender::new( | ||
| &format!("nh os {variant}"), | ||
| "Apply the config?", | ||
| ) | ||
| .ask() | ||
| }, | ||
| NotifyAskMode::Both => unimplemented!(), | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Heads-up: notification path may panic inside notify backend
NotificationSender::ask() in notify.rs calls show().unwrap(). Any failure to display notifications will abort here. Consider converting ask() to return Result<bool> and falling back to a terminal prompt on error.
You can grep for potential panics/unimplemented across the repo:
Also applies to: 474-492
🏁 Script executed:
#!/bin/bash
rg -n "unimplemented!\(" -g '!**/target/**'
rg -nP -C2 'show\(\)\.unwrap\(\)|\.ask\(\)\.unwrap\(\)' -g '!**/target/**'Length of output: 245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching repository for unimplemented! and unwrap() usages, NotificationSender, and notify.rs..."
echo
echo "1) occurrences of unimplemented!("
rg -nF --hidden -uu -g '!**/target/**' 'unimplemented!(' || true
echo
echo "2) literal .show().unwrap() and .ask().unwrap()"
rg -nF --hidden -uu -g '!**/target/**' '.show().unwrap()' || true
rg -nF --hidden -uu -g '!**/target/**' '.ask().unwrap()' || true
echo
echo "3) NotificationSender definitions/impls (context) and all .ask() call sites (context)"
rg -n --hidden -uu -C3 -g '!**/target/**' 'NotificationSender' || true
rg -n --hidden -uu -C3 -g '!**/target/**' '\.ask\(\)' || true
echo
echo "4) locate notify.rs files"
find . -type f -name 'notify.rs' -print || true
echo
echo "5) print notify.rs contents (first 400 lines) if present"
while IFS= read -r f; do
echo "---- $f ----"
sed -n '1,400p' "$f"
done < <(find . -type f -name 'notify.rs' -print) || trueLength of output: 1174
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== cat src/notify.rs ==="
if [ -f src/notify.rs ]; then
sed -n '1,240p' src/notify.rs
else
echo "src/notify.rs not found"
fi
echo
echo "=== search for unimplemented! occurrences ==="
rg -nF --hidden -uu 'unimplemented!' || true
echo
echo "=== search for .show().unwrap() and .ask().unwrap() occurrences (literal) ==="
rg -nF --hidden -uu '.show().unwrap()' || true
rg -nF --hidden -uu '.ask().unwrap()' || true
echo
echo "=== search NotificationSender and .ask( occurrences with context ==="
rg -n --hidden -uu -C3 'NotificationSender' || true
rg -n --hidden -uu -C3 '.ask(' || true
echo
echo "=== cat src/nixos.rs (first 800 lines) ==="
if [ -f src/nixos.rs ]; then
sed -n '1,800p' src/nixos.rs
else
echo "src/nixos.rs not found"
fiLength of output: 33719
Heads-up: NotificationSender::ask() can panic — make it return Result and handle failures; stop unwrapping notification send.
- Problem: src/notify.rs calls self.notification.show().unwrap() (line ~105) so a notification backend failure will panic.
- Fix: change ask() to return Result (or handle the error internally) and on error fall back to a terminal prompt / non-interactive default. Update callers to handle Err instead of assuming bool.
- Call sites to update: src/nixos.rs (notify branches around the rebuild/rollback prompts), src/home.rs (notify path), src/clean.rs (notify path).
- Related: logging macros call NotificationSender::send().unwrap() (src/logging.rs lines ~99,111,122,133) — replace these unwraps with graceful handling (.send().ok() or log/send error) to avoid panics.
🤖 Prompt for AI Agents
In src/nixos.rs around lines 281-296 the code assumes NotificationSender::ask()
returns a bool and will never fail; change the notification path to handle a
Result<bool> from ask() (or handle the internal error) so notification backend
failures don't panic: update NotificationSender::ask() to return Result<bool, E>
(or Result<bool, anyhow::Error>) and in the match arms call .ask() and match on
Ok(true)/Ok(false)/Err(_) — on Err(_) fall back to a terminal prompt (in
interactive mode) or a safe non-interactive default (e.g., false) and log the
error; update the other call sites (src/home.rs and src/clean.rs) similarly to
handle Err instead of unwrapping, and replace all
NotificationSender::send().unwrap() usages in src/logging.rs with non-panicking
handling (.send().ok() or log the error) so notification failures are handled
gracefully.
5a80cac to
29f4862
Compare
Change-Id: I6a6a69642ac3443525df583fd200827414801d41
r0chd
force-pushed
the
notifications
branch
from
29f4862 to
34f38e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/darwin.rs (1)
158-176: Don’t unwrap notification send; use meaningful body and keep notify-then-promptUnwrap can panic if no server is available; also replace “testing” with the actual question. Keep semantics: notify (best-effort) then prompt.
- // MacOS doesn't support notification actions - NotifyAskMode::Notify | NotifyAskMode::Both => { - NotificationSender::new(&format!("nh darwin {variant}"), "testing") - .send() - .unwrap(); - - inquire::Confirm::new("Apply the config?") - .with_default(false) - .prompt()? - }, + // macOS: no actions; notify then prompt + NotifyAskMode::Notify | NotifyAskMode::Both => { + if let Err(e) = NotificationSender::new( + &format!("nh darwin {variant}"), + "Apply the config?", + ).send() { + debug!(?e, "Notification failed; continuing with prompt"); + } + inquire::Confirm::new("Apply the config?") + .with_default(false) + .prompt()? + },src/nixos.rs (2)
474-492: Same issues on rollback: unimplemented! and panic riskMirror the rebuild fix: implement Both and ensure ask path won’t panic when notifications fail.
NotifyAskMode::Notify => { NotificationSender::new( "nh os rollback", &format!("Roll back to generation {}?", target_generation.number), ) - .ask() + .ask() }, - NotifyAskMode::Both => unimplemented!(), + NotifyAskMode::Both => { + let _ = NotificationSender::new( + "nh os rollback", + &format!("Roll back to generation {}?", target_generation.number), + ).send(); + inquire::Confirm::new(&format!( + "Roll back to generation {}?", + target_generation.number + )) + .with_default(false) + .prompt()? + },
281-296: Unimplemented! in user path and potential panic via ask()
NotifyAskMode::Both => unimplemented!()will crash on--ask=both.NotificationSender::ask()currently callsshow().unwrap()(see src/notify.rs), which can panic if no server is running.Implement Both as “notify (best-effort) then prompt” and rely on a non-panicking ask() (see proposed fix in src/notify.rs).
NotifyAskMode::Notify => { NotificationSender::new( &format!("nh os {variant}"), "Apply the config?", ) - .ask() + .ask() }, - NotifyAskMode::Both => unimplemented!(), + NotifyAskMode::Both => { + let _ = NotificationSender::new( + &format!("nh os {variant}"), + "Apply the config?", + ).send(); + inquire::Confirm::new("Apply the config?") + .with_default(false) + .prompt()? + },src/notify.rs (1)
100-127: ask(): avoid panic; don’t call show() on macOS; make errors non-fatalCurrent
show().unwrap()will abort on headless/minimal systems and is executed even on macOS. Restructure under cfgs and handle errors by returningfalse, letting callers fall back to the terminal prompt.- pub fn ask(mut self) -> bool { - #[cfg(all(unix, not(target_os = "macos")))] - { - self - .notification - .urgency(self.urgency.unwrap_or(Urgency::Critical)); - self.notification.action("accept", "Accept"); - self.notification.action("reject", "Reject"); - } - - let handle = self.notification.show().unwrap(); - - #[cfg(all(unix, not(target_os = "macos")))] - { - let mut confirmation = false; - handle.wait_for_action(|s| { - confirmation = s == "accept"; - }); - confirmation - } - - #[cfg(target_os = "macos")] - { - _ = &mut self; - _ = handle; - false - } - } + pub fn ask(mut self) -> bool { + // macOS: notify-rust does not support actionable notifications; let caller fall back. + #[cfg(target_os = "macos")] + { + let _ = &mut self; + return false; + } + + // Unix (non-macOS): actionable notification with Accept/Reject + #[cfg(all(unix, not(target_os = "macos")))] + { + self + .notification + .urgency(self.urgency.unwrap_or(Urgency::Critical)); + self.notification.action("accept", "Accept"); + self.notification.action("reject", "Reject"); + + let handle = match self.notification.show() { + Ok(h) => h, + Err(_e) => return false, // server unavailable; let caller fall back + }; + + let mut confirmation = false; + handle.wait_for_action(|s| { + confirmation = s == "accept"; + }); + return confirmation; + } + + // Other targets: unsupported + false + }
🧹 Nitpick comments (4)
src/interface.rs (3)
259-267: Tri-state ask mode: solid addition; consider CopyEnum looks good and documented. Derive Copy to avoid needless moves for a tiny POD.
-#[derive(Debug, Clone, ValueEnum, PartialEq)] +#[derive(Debug, Clone, Copy, ValueEnum, PartialEq)] pub enum NotifyAskMode {
276-278: Clap attribute consistency for --askYou use
value_enumhere but not on the otheraskoptions. Clap infersValueEnumfrom the type, so drop it here for consistency and clearer help output.- #[arg(long, short, value_enum, default_missing_value = "prompt", num_args = 0..=1)] + #[arg(long, short, default_missing_value = "prompt", num_args = 0..=1)] pub ask: Option<NotifyAskMode>,
440-442: Align ask flags across subcommandsSame note as above: ensure the help text shows possible values uniformly across commands; with the change in OsRollbackArgs, all three will behave the same.
src/notify.rs (1)
60-78: send(): ok; consider minor polishOptional: set app name and (if desired) a cross‑platform icon; keep behind env if you prefer.
- self.notification.show()?; + // Optionally tag the app name for some servers: + // self.notification.appname("nh"); + self.notification.show()?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/clean.rs(2 hunks)src/commands.rs(3 hunks)src/darwin.rs(4 hunks)src/generations.rs(2 hunks)src/home.rs(5 hunks)src/interface.rs(3 hunks)src/logging.rs(1 hunks)src/nixos.rs(7 hunks)src/notify.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/generations.rs
- src/logging.rs
- src/commands.rs
- src/home.rs
- src/clean.rs
🧰 Additional context used
🧬 Code graph analysis (3)
src/darwin.rs (1)
src/notify.rs (2)
ask(100-127)new(25-33)
src/nixos.rs (3)
src/darwin.rs (1)
fmt(54-60)src/home.rs (1)
fmt(53-59)src/notify.rs (2)
ask(100-127)new(25-33)
src/notify.rs (1)
src/commands.rs (3)
new(160-171)new(601-609)new(723-732)
⏰ 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). (5)
- GitHub Check: Test NH on Darwin
- GitHub Check: Build NH on Linux
- GitHub Check: Build NH on Darwin
- GitHub Check: Test NH on Linux
- GitHub Check: treewide-checks
🔇 Additional comments (7)
src/interface.rs (1)
308-310: Back-compat semantics check for tri‑state --askConfirm that bare
--askyieldsSome(Prompt)(viadefault_missing_value = "prompt") and absence of the flag yieldsNone(back-compat: no prompt by default). If that’s intended across commands, we’re good.src/darwin.rs (2)
38-41: Correct: warn that --ask/--dry don’t affect buildClear UX. No issues.
53-61: Nice: Display impl for variantHelps keep messages consistent.
src/nixos.rs (4)
45-49: Correct: warn that --ask/--dry don’t affect buildMatches darwin; reduces confusion.
67-78: Good: Display impl for variantKeeps user-facing strings tidy.
274-279: Dry/build gating message is rightWarning on
--askin dry/build paths is appropriate.
404-405: LGTM: use nh_debug! for final pathConsistent with new logging macros.
Implemented system notifications with dbus notification spec, notification is sent right before
password prompt, also implemented
--askflag with notification actionsits currently implemented only for nixos, looking for some input before implementing more for interfaces
nix fmtto format my Nix codecargo fmtto format my Rust codecargo clippyand fixed any new linter warnings.logic
description.
x86_64-linuxaarch64-linuxx86_64-darwinaarch64-darwinAdd a 👍 reaction to pull requests you find important.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Style