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 via notify-rust, introduces a tri-state Ask mode ( Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant Command
participant AskMode as NotifyAskMode
participant Notifier as NotificationSender
participant Tracing
User->>CLI: run command (maybe `--ask`)
CLI->>Command: parse args -> Option<NotifyAskMode>
alt ask is None
Command->>Command: proceed without confirmation
else ask is Some
Command->>AskMode: match variant
alt Prompt
Command->>User: terminal prompt (confirm)
User-->>Command: accept/reject
else Notify
Command->>Notifier: new(summary, body).urgency(Critical)
Notifier->>Notifier: show notification (actions on unix)
alt unix && not macos
User-->>Notifier: accept/reject action
Notifier-->>Command: returns bool
else macos or unsupported
Notifier-->>Command: returns false (no interactive ask)
end
else Both
Note right of Command: path currently unimplemented
end
end
Command->>Tracing: nh_info!/nh_warn! (logs + notification)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
🚧 Files skipped from review as they are similar to previous changes (5)
🧰 Additional context used🧬 Code graph analysis (4)src/home.rs (1)
src/nixos.rs (1)
src/logging.rs (1)
src/clean.rs (1)
⏰ 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)
🔇 Additional comments (15)
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.
src/commands.rs
Outdated
| 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.
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.
src/home.rs
Outdated
| 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.
src/logging.rs
Outdated
| #[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.
src/logging.rs
Outdated
| #[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.
r0chd
force-pushed
the
notifications
branch
from
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.
r0chd
force-pushed
the
notifications
branch
3 times, most recently
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.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/util/platform.rs (1)
275-286: Compile error: usingselfin a free function; pass the localpassthrough.
.passthrough(&self.passthrough)won’t compile (noselfhere). Use the variable computed at Line 275.Apply:
- .passthrough(&self.passthrough) + .passthrough(&passthrough)
♻️ Duplicate comments (5)
src/notify.rs (2)
100-127: Don’t unwrap; avoid calling show() on macOS; propagate errors and gate by NH_NOTIFY.
show().unwrap()will panic on systems without a notification server; macOS path still evaluatesshow()before returning. Reorder cfg blocks, return early on macOS, and use?to propagate errors. Also consider honoring NH_NOTIFY likesend()does.Apply:
- 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) -> Result<bool> { + // Respect global toggle, like `send()` + let enable_notifications = env::var("NH_NOTIFY").is_ok_and(|v| v == "1"); + if !enable_notifications { + return Ok(false); + } + + #[cfg(target_os = "macos")] + { + // Actions not supported on macOS via notify-rust backend. + return Ok(false); + } + + #[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"; + }); + return Ok(confirmation); + } + + #[allow(unreachable_code)] + Ok(false) + }Optional: set
.appname("nh")and consider a neutral icon if maintainers approve. Based on learnings.
80-99: Doc/signature mismatch in ask(); adjust API to return Result or fix docs.Docs say “Returns an error if the notification cannot be shown” and describe Ok(true)/Ok(false), but the function returns bool. This prevents callers from falling back cleanly on transport errors. Recommend
pub fn ask(self) -> Result<bool>.Apply:
- /// # Errors - /// - /// Returns an error if the notification cannot be shown. - #[must_use] - pub fn ask(mut self) -> bool { + /// # Errors + /// + /// Returns an error if the notification cannot be shown. + pub fn ask(mut self) -> Result<bool> {src/clean.rs (2)
337-353: Removeunimplemented!()for Both; implement “notify, else prompt” fallback.Leaving
Bothunimplemented will panic at runtime. Implement: try notification; if it returns Err orfalse, fall back to terminal prompt.Apply:
- NotifyAskMode::Both => unimplemented!(), + NotifyAskMode::Both => { + match NotificationSender::new( + "nh clean", + "Do you want to confirm the cleanup plan?", + ) + .ask() { + Ok(true) => true, + // rejected, dismissed, unsupported, or failed -> prompt + _ => { + Confirm::new("Confirm the cleanup plan?") + .with_default(false) + .prompt()? + } + } + },Note: depends on notify::ask() returning Result as proposed.
345-351: Handle notification transport errors; don’t assume success.Update the Notify path to handle
Result<bool>and fall back on errors.Apply:
- NotifyAskMode::Notify => { - NotificationSender::new( - "nh clean", - "Do you want to confirm the cleanup plan?", - ) - .ask() - }, + NotifyAskMode::Notify => match NotificationSender::new( + "nh clean", + "Do you want to confirm the cleanup plan?", + ).ask() { + Ok(v) => v, // true = accepted; false = rejected/dismissed/unsupported + Err(_e) => { + // Optional: debug!(?e, "Notification failed; falling back to prompt"); + Confirm::new("Confirm the cleanup plan?") + .with_default(false) + .prompt()? + } + },src/nixos.rs (1)
470-488: Critical: Same panic and unimplemented issues in rollback confirmation.Identical problems as the rebuild path (lines 293-308):
NotificationSender::ask()panics internally on notification failuresNotifyAskMode::Both => unimplemented!()will crash- No fallback when notifications fail
Apply the same fix pattern as suggested for rebuild:
if let Some(ask) = &self.ask { 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() + match NotificationSender::new( + "nh os rollback", + &format!("Roll back to generation {}?", target_generation.number), + ).ask() { + Ok(confirmed) => confirmed, + 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 => 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 (2)
src/notify.rs (1)
9-11: Minor tidy-ups: use imported type and set app name (optional).
- Field can use imported
Notificationinstead of fully-qualified path.- Consider
.appname("nh")beforeshow()to improve UX consistency.Apply:
- notification: notify_rust::Notification, + notification: Notification,And (near send()):
- self.notification.show()?; + self.notification.appname("nh").show()?;Also applies to: 66-76
src/clean.rs (1)
360-399: Optional: skip prompting in dry-run for consistency with other flows.Consider skipping confirmations when
args.dryis true, mirroring platform::confirm_action().- if let Some(ask) = &args.ask { + if !args.dry { + if let Some(ask) = &args.ask { /* ... existing confirmation logic ... */ - } + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.toml(1 hunks)src/clean.rs(3 hunks)src/commands.rs(4 hunks)src/darwin.rs(4 hunks)src/home.rs(3 hunks)src/interface.rs(3 hunks)src/lib.rs(1 hunks)src/logging.rs(1 hunks)src/main.rs(1 hunks)src/nixos.rs(12 hunks)src/notify.rs(1 hunks)src/util.rs(2 hunks)src/util/platform.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/main.rs
- src/logging.rs
- Cargo.toml
- src/home.rs
- src/darwin.rs
- src/commands.rs
🧰 Additional context used
🧬 Code graph analysis (3)
src/nixos.rs (1)
src/notify.rs (2)
ask(100-127)new(25-33)
src/clean.rs (1)
src/notify.rs (2)
ask(100-127)new(25-33)
src/notify.rs (1)
src/commands.rs (3)
new(164-175)new(619-627)new(741-750)
🪛 GitHub Actions: Check formating & lints
src/util.rs
[error] 1-1: Rust fmt check reported formatting changes in this file. Run 'cargo fmt' to fix.
⏰ 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 Linux
- GitHub Check: Test NH on Darwin
- GitHub Check: Build NH on Darwin
- GitHub Check: Test NH on Linux
🔇 Additional comments (9)
src/util/platform.rs (1)
189-196: LGTM on switching to nh_info!Switching the prompt preface to
nh_info!aligns with the new logging path.src/lib.rs (1)
14-15: Module exposure looks good.
pub mod notify;is correctly added; no further issues.src/util.rs (2)
329-333: LGTM on log macro change.
nh_info!improves consistency with the new notification-integrated logging.
1-1: Runcargo fmtto fix formatting issues in this file.File
src/util.rsuses 2-space indentation throughout (lines 1–40+), but the standard Rust formatter expects 4-space indentation. Executecargo fmt --alllocally and push the changes.src/clean.rs (1)
522-526: LGTM on nh_info! for removals.User-facing message is appropriate and consistent.
src/nixos.rs (3)
8-8: LGTM: Import changes align with the new notification infrastructure.The removal of
infofrom tracing and addition ofnh_info!,NotifyAskMode, andNotificationSendercorrectly support the tri-state confirmation system.Also applies to: 18-18, 26-27
45-45: LGTM: Option handling is correct.The
is_some()checks correctly handle the newOption<NotifyAskMode>type while preserving the original warning logic.Also applies to: 281-281
425-425: LGTM: nh_info! macro usage is appropriate.The replacement of
tracing::info!withnh_info!correctly surfaces important user-facing messages (progress updates, success confirmations, instructions) while integrating with the notification system.Also applies to: 463-466, 505-505, 537-537, 562-565, 671-682, 705-708
src/interface.rs (1)
267-275: LGTM: Enum definition is well-structured.The
NotifyAskModeenum with its three variants (Prompt, Notify, Both) is properly defined with appropriate derives and clear documentation. Note that theBothvariant implementation remains incomplete in usage sites (see comments on src/nixos.rs).
| #[arg(long, short, value_enum, default_missing_value = "prompt", num_args = 0..=1)] | ||
| pub ask: Option<NotifyAskMode>, |
There was a problem hiding this comment.
🧩 Analysis chain
Critical: Missing value_enum attribute causes CLI parsing failures.
OsRollbackArgs.ask (line 284) includes value_enum in its clap attribute, but CommonRebuildArgs.ask (line 316) and CleanArgs.ask (line 448) do not. Without value_enum, clap cannot parse the enum variants from the command line, so --ask=notify and --ask=both will fail with a parsing error.
Apply this fix:
/// Ask for confirmation
- #[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>,for both CommonRebuildArgs (line 316) and CleanArgs (line 448).
Run this script to verify all ask fields consistently use value_enum:
Also applies to: 316-317, 448-449
🏁 Script executed:
#!/bin/bash
# Find all ask field declarations and their attributes
rg -n -A1 -B1 'pub ask: Option<NotifyAskMode>' src/interface.rs
# Check which ones have value_enum in their clap attribute
rg -n -B2 'pub ask: Option<NotifyAskMode>' src/interface.rs | grep -E '(#\[arg|pub ask)'Length of output: 740
🏁 Script executed:
#!/bin/bash
# Find NotifyAskMode definition and its traits/derives
rg -n -A10 'enum NotifyAskMode' src/interface.rs
# Check if NotifyAskMode is derived with any trait that enables FromStr
rg -n -B5 'enum NotifyAskMode' src/interface.rs | head -20Length of output: 440
🏁 Script executed:
#!/bin/bash
# Search for any FromStr implementation for NotifyAskMode
rg -n 'impl.*FromStr.*NotifyAskMode|NotifyAskMode.*FromStr' src/
# Also search for clap derive or value_enum usage patterns
rg -n '#\[derive.*Clap|#\[derive.*ValueEnum' src/interface.rs | head -10Length of output: 136
🏁 Script executed:
#!/bin/bash
# Search for tests exercising the --ask flag
rg -n '\-\-ask|ask.*Option.*NotifyAskMode' tests/ --type rust 2>/dev/null | head -20
# Also check if there's any test or integration test
fd -e rs tests/ 2>/dev/null | head -10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check clap's ValueEnum behavior - verify that derived ValueEnum works without explicit value_enum attribute
# Search for any documentation or comments about this pattern
rg -n 'ValueEnum|value_enum' src/interface.rs | head -20Length of output: 438
Add missing value_enum attribute to fix CLI parsing failures.
The verification confirms the issue: NotifyAskMode derives ValueEnum, but the value_enum attribute in the #[arg(...)] macro is still required to enable proper enum parsing. Without it, clap cannot parse enum variants from command-line arguments.
- Line 284 (OsRollbackArgs): ✓ Correct, has
value_enum - Line 316 (CommonRebuildArgs): ✗ Missing
value_enum - Line 448 (CleanArgs): ✗ Missing
value_enum
Apply this fix to lines 316 and 448:
- #[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>,Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/interface.rs around lines 316 and 448, the #[arg(...)] macros for the
fields using NotifyAskMode are missing the value_enum attribute, which prevents
clap from parsing enum variants even though NotifyAskMode derives ValueEnum;
update those two arg attributes to include value_enum (matching the one at line
284) so the enum is parsed correctly from CLI arguments.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
src/clean.rs (1)
338-353: Implement safe ask flow and define Both semantics; avoid panics
Current Notify path depends on ask() panicking on delivery failure, and Both is unimplemented. Switch to a Result-based ask() and implement “notify, else prompt” fallback.- if let Some(ask) = &args.ask { - let confirmation = match ask { - NotifyAskMode::Prompt => { - Confirm::new("Confirm the cleanup plan?") - .with_default(false) - .prompt()? - }, - NotifyAskMode::Notify => { - NotificationSender::new( - "nh clean", - "Do you want to confirm the cleanup plan?", - ) - .ask() - }, - NotifyAskMode::Both => unimplemented!(), - }; + if let Some(ask) = &args.ask { + 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()? + } + } + }, + };Note: requires NotificationSender::ask() -> Result. Based on learnings.
src/logging.rs (1)
98-118: Don’t unwrap in macros; use $crate path and handle errors; support structured fields
Unwrap can crash when no server. Also format!($($arg)*) fails for structured-field calls. Handle errors and add a literal-first arm.#[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(); - }}; + // literal message + optional fields + ($fmt:literal $(, $fields:tt)*) => {{ + let message = format!($fmt $(, $fields)*); + tracing::info!($fmt $(, $fields)*); + 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"); + } + }}; + // fallback: just forward to tracing (avoids format! on non-literals) + ($($arg:tt)*) => {{ + tracing::info!($($arg)*); + }}; } #[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(); - }}; + ($fmt:literal $(, $fields:tt)*) => {{ + let message = format!($fmt $(, $fields)*); + tracing::warn!($fmt $(, $fields)*); + 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"); + } + }}; + ($($arg:tt)*) => {{ + tracing::warn!($($arg)*); + }}; }src/home.rs (1)
168-183: Use Result-based ask with fallback; implement Both
Mirror clean.rs: notify ask with explicit Accept; on Err, fall back to prompt; Both = 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 home switch", - "Do you want to apply the Home Manager configuration?", - ) - .ask() - }, - 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( + "nh home switch", + "Apply the Home Manager configuration?", + ).ask() { + Ok(true) => true, + Ok(false) => false, + 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( + "nh home switch", + "Apply the Home Manager configuration?", + ).ask() { + Ok(true) => true, + _ => { + inquire::Confirm::new("Apply the config?") + .with_default(false) + .prompt()? + } + } + }, + };Requires NotificationSender::ask() -> Result. Based on learnings.
src/notify.rs (1)
100-127: ask() must not panic and should signal failure to callers
show().unwrap() will crash when no notification server is present; macOS path still calls show(). Return Result, skip show() on macOS, and support action flow only on Unix.- #[must_use] - 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) -> Result<bool> { + // Respect NH_NOTIFY like send(); disabled => let caller fall back. + let enable = matches!(env::var("NH_NOTIFY").as_deref(), Ok("1")); + if !enable { + return Err(color_eyre::eyre::eyre!("Notifications disabled (NH_NOTIFY!=1)")); + } + + #[cfg(target_os = "macos")] + { + // Actions unsupported via notify-rust on macOS; trigger fallback. + let _ = &mut self; + return Ok(false); + } + + #[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 delivery error + let mut confirmation = false; + handle.wait_for_action(|s| { + confirmation = s == "accept"; + }); + Ok(confirmation) + } + }src/darwin.rs (1)
166-175: Critical: Unwrap can panic and placeholder text remains.Two blocking issues flagged in previous reviews but still present:
- Line 170:
.unwrap()will crash if the notification fails (missing daemon, dismissed notification, etc.).- Line 168: Body text "testing" is a placeholder and not user-appropriate.
Per past reviews, these were marked as addressed in commits def8fe0–7c65e7b, but the issues remain in the current code.
Apply this diff:
// MacOS doesn't support notification actions NotifyAskMode::Notify | NotifyAskMode::Both => { - NotificationSender::new(&format!("nh darwin {variant}"), "testing") - .send() - .unwrap(); + 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)
293-308: Critical: Notification can panic and Both mode is unimplemented.Three blocking issues previously flagged but still present:
Panic risk (lines 300–305):
NotificationSender::ask()calls.show().unwrap()internally (see src/notify.rs line ~110). If the notification daemon is unavailable or the call fails, the application crashes instead of falling back to a terminal prompt.Unimplemented Both (line 307):
NotifyAskMode::Both => unimplemented!()will panic if users pass--ask=both. Per PR discussion, this mode should show both notification and terminal prompt.No error recovery: The Notify branch assumes ask() always succeeds, leaving no fallback when notifications fail.
Recommended fix:
Change
NotificationSender::ask()to returnResult<bool>(in src/notify.rs), then update callers: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 switch", - "Do you want to apply the NixOS configuration?", - ) - .ask() + match NotificationSender::new( + "nh os switch", + "Do you want to apply the NixOS configuration?", + ).ask() { + Ok(confirmed) => confirmed, + Err(e) => { + warn!("Notification prompt failed: {e}; falling back to terminal"); + inquire::Confirm::new("Apply the config?") + .with_default(false) + .prompt()? + } + } }, - NotifyAskMode::Both => unimplemented!(), + NotifyAskMode::Both => { + let _ = NotificationSender::new( + "nh os switch", + "Do you want to apply the NixOS configuration?", + ).send(); + inquire::Confirm::new("Apply the config?") + .with_default(false) + .prompt()? + }, };
470-488: Critical: Rollback mirrors rebuild confirmation issues.This rollback confirmation path has the same three critical issues as the rebuild confirmation (lines 293–308):
ask()can panic if notification fails (line 485)Bothmode is unimplemented (line 487)- No error recovery or fallback to terminal prompt
Apply the same fix pattern as recommended for the rebuild confirmation above, adjusting the notification body to reference the rollback generation.
🧹 Nitpick comments (5)
src/clean.rs (1)
523-524: Use of nh_info! is fine; ensure macros don’t panic
This site is OK, but nh_info! currently unwraps send(); address in src/logging.rs to avoid crash here if notifications fail.src/notify.rs (2)
60-66: MSRV-friendly NH_NOTIFY gating
is_ok_and may raise MSRV; prefer matches! to keep builds wide.- let enable_notifications = env::var("NH_NOTIFY").is_ok_and(|v| v == "1"); + let enable_notifications = matches!(env::var("NH_NOTIFY").as_deref(), Ok("1"));
25-33: Set app name (optional)
Setting appname improves UX in some DEs.- notification.summary(summary); + notification.summary(summary).appname("nh");src/home.rs (1)
191-195: OK to use nh_info!; ensure macro fix lands
This site is fine once macros avoid unwrap.src/util.rs (1)
191-195: Remove unnecessary unsafe around env::set_var
set_var is safe; drop the unsafe block.// before unsafe { env::set_var("HOME_MANAGER_BACKUP_EXT", ext); } // after env::set_var("HOME_MANAGER_BACKUP_EXT", ext);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/clean.rs(3 hunks)src/darwin.rs(4 hunks)src/home.rs(3 hunks)src/interface.rs(3 hunks)src/logging.rs(2 hunks)src/nixos.rs(13 hunks)src/notify.rs(1 hunks)src/util.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/interface.rs
🧰 Additional context used
🧬 Code graph analysis (5)
src/darwin.rs (1)
src/notify.rs (2)
ask(100-127)new(25-33)
src/logging.rs (1)
src/notify.rs (3)
new(25-33)urgency(41-44)send(60-78)
src/nixos.rs (1)
src/notify.rs (2)
ask(100-127)new(25-33)
src/clean.rs (1)
src/notify.rs (2)
ask(100-127)new(25-33)
src/home.rs (1)
src/notify.rs (2)
ask(100-127)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). (2)
- GitHub Check: Test NH on Linux
- GitHub Check: Build NH on Linux
🔇 Additional comments (7)
src/logging.rs (1)
34-43: Formatter changes look good
Using write!(..)? keeps error propagation correct.src/home.rs (1)
38-41: Dry-run/ask warning is appropriate
Message clearly informs users build ignores ask/dry.src/util.rs (1)
333-333: Switch to nh_info! is fine
Once macros are made non-panicking, this is safe.src/darwin.rs (2)
1-22: LGTM: Imports are correct.The added imports (
fmt,NotifyAskMode,NotificationSender) properly support the new notification and tri-state ask features.
53-61: LGTM: Clean Display implementation.The Display trait implementation is correct and enables user-friendly variant names in notifications.
src/nixos.rs (2)
1-30: LGTM: Imports support the new notification features.The additions of
NotifyAskMode,NotificationSender, andnh_info!properly enable the tri-state ask mode and notification-based logging.
425-425: LGTM: Consistent logging macro migration.Replacing
info!withnh_info!throughout the file aligns with the notification-based logging system introduced in this PR. The usage is consistent and the messages are appropriate for user-facing notifications.Also applies to: 463-466, 505-505, 537-537, 562-565, 671-674, 678-682, 705-708
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
src/logging.rs (2)
98-107: format!($($arg)*) blocks structured fieldsCalls like nh_info!(%val, key=?x) won’t compile. Either document “literal‑first only” or add a literal‑only arm plus a fallback arm that only notifies a static message.
Example sketch:
#[macro_export] macro_rules! nh_info { - ($($arg:tt)*) => {{ + ($fmt:literal $(, $arg:tt)*) => {{ let message = format!($fmt $(, $arg)*); - tracing::info!($($arg)*); + tracing::info!($fmt $(, $arg)*); // send... }}; + ($($fields:tt)+) => {{ + tracing::info!($($fields)+); + // Optional: skip or send a generic notification body here. + }} }#!/bin/bash # List non-literal macro uses; should be empty or intentionally allowed rg -nP 'nh_(info|warn)!\(\s*"' -v src | sed -n '1,120p'Also applies to: 109-118
98-107: Don’t panic in logging macros; handle send() errors.send().unwrap() can crash when notifications fail. Log and continue instead.
Apply:
#[macro_export] macro_rules! nh_info { ($($arg:tt)*) => {{ - use notify_rust::Urgency; - use $crate::notify::NotificationSender; + use ::notify_rust::Urgency; let message = format!($($arg)*); tracing::info!($($arg)*); - NotificationSender::new("nh info", &message).urgency(Urgency::Normal).send().unwrap(); + if let Err(e) = $crate::notify::NotificationSender::new("nh info", &message) + .urgency(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; + use ::notify_rust::Urgency; let message = format!($($arg)*); tracing::warn!($($arg)*); - NotificationSender::new("nh warn", &message).urgency(Urgency::Normal).send().unwrap(); + if let Err(e) = $crate::notify::NotificationSender::new("nh warn", &message) + .urgency(Urgency::Normal) + .send() + { + tracing::debug!(?e, "Failed to send nh warn notification"); + } }}; }Also applies to: 109-118
src/interface.rs (1)
316-318: Add value_enum to fix enum parsing for --askCommonRebuildArgs.ask and CleanArgs.ask are missing value_enum; clap won’t parse --ask=notify|both without it. Align with OsRollbackArgs.
Apply:
- #[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>,#!/bin/bash # Verify all NotifyAskMode ask fields include value_enum rg -n -C1 'pub ask:\s*Option<NotifyAskMode>' src/interface.rsAlso applies to: 448-450
src/darwin.rs (1)
158-176: macOS: notify then prompt; avoid unwrap and fix textUse a meaningful body and don’t unwrap send(); log and proceed to 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}"), + "Do you want to apply the Darwin configuration?", + ).send() { + debug!(?e, "Notification failed; continuing with prompt"); + } + inquire::Confirm::new("Apply the config?") + .with_default(false) + .prompt()? + },src/clean.rs (1)
338-357: Implement Both and handle notify failures (no panic)Both is unimplemented and Notify assumes infallible ask(). Add notify‑then‑prompt fallback.
- if let Some(ask) = &args.ask { - let confirmation = match ask { + if let Some(ask) = &args.ask { + let confirmation = match ask { NotifyAskMode::Prompt => { Confirm::new("Confirm the cleanup plan?") .with_default(false) .prompt()? }, - NotifyAskMode::Notify => { - NotificationSender::new( - "nh clean", - "Do you want to confirm the cleanup plan?", - ) - .ask() - }, - NotifyAskMode::Both => unimplemented!(), + NotifyAskMode::Notify => { + match NotificationSender::new( + "nh clean", + "Do you want to 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 => { + match NotificationSender::new( + "nh clean", + "Do you want to confirm the cleanup plan?", + ).ask() { + Ok(true) => true, + _ => Confirm::new("Confirm the cleanup plan?") + .with_default(false) + .prompt()? + } + }, };Optional: skip ask when args.dry for consistency with home/darwin.
src/home.rs (1)
168-183: Implement safe Notify and Both flows; enable fallbackBoth panics (unimplemented) and Notify assumes infallible ask(). Make ask notify‑then‑prompt with error handling.
Apply (requires notify.rs ask() -> Result):
- 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 home switch", - "Do you want to apply the Home Manager configuration?", - ) - .ask() - }, - NotifyAskMode::Both => unimplemented!(), + NotifyAskMode::Notify => { + match NotificationSender::new( + "nh home switch", + "Do you want to apply the Home Manager configuration?", + ).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 => { + match NotificationSender::new( + "nh home switch", + "Do you want to apply the Home Manager configuration?", + ).ask() { + Ok(true) => true, + _ => inquire::Confirm::new("Apply the config?") + .with_default(false) + .prompt()? + } + }, };Follow‑up in src/notify.rs: return Result from ask() and propagate show()/wait errors.
src/nixos.rs (2)
470-488: Critical: Apply the same fixes to rollback confirmation.This rollback confirmation path has identical issues to the rebuild flow (lines 293-308):
NotificationSender::ask()can panic internallyBothmode will crash withunimplemented!()- No fallback when notification delivery fails
Apply the same error-handling pattern documented in the rebuild comment, ensuring the notification body includes the generation number as shown here.
293-308: Critical: Notification path can panic, and Both mode is unimplemented.Three blocking issues remain despite past review feedback:
Panic risk:
NotificationSender::ask()internally calls.show().unwrap()(see src/notify.rs line ~110). When the notification daemon is unavailable or fails, the application will crash instead of gracefully degrading.Unimplemented Both mode:
NotifyAskMode::Both => unimplemented!()will panic if users pass--ask=both. Per the PR discussion, this mode should display both a notification and require terminal confirmation.No error recovery: The
Notifybranch assumesask()always succeeds, leaving users with no way to confirm when notifications fail.Required fixes:
First, update
src/notify.rsto makeask()returnResult<bool>and remove internal.unwrap()calls. Then apply this pattern: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 switch", - "Do you want to apply the NixOS configuration?", - ) - .ask() + // Attempt notification prompt; fall back to terminal on failure + match NotificationSender::new( + "nh os switch", + "Do you want to apply the NixOS configuration?", + ).ask() { + Ok(confirmed) => confirmed, + Err(e) => { + warn!("Notification prompt failed: {e}; falling back to terminal prompt"); + inquire::Confirm::new("Apply the config?") + .with_default(false) + .prompt()? + } + } }, - NotifyAskMode::Both => unimplemented!(), + NotifyAskMode::Both => { + // Show notification (best-effort) then require terminal confirmation + let _ = NotificationSender::new( + "nh os switch", + "Do you want to apply the NixOS configuration?", + ).send(); + inquire::Confirm::new("Apply the config?") + .with_default(false) + .prompt()? + }, };Note: The same pattern must be applied to the rollback flow (lines 470-488) and any other call sites in home.rs and clean.rs mentioned in past reviews.
🧹 Nitpick comments (1)
src/home.rs (1)
190-195: Remove unnecessary unsafe around set_varstd::env::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);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Cargo.toml(2 hunks)src/clean.rs(3 hunks)src/darwin.rs(4 hunks)src/home.rs(3 hunks)src/interface.rs(3 hunks)src/logging.rs(2 hunks)src/nixos.rs(13 hunks)src/notify.rs(1 hunks)src/util.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/util.rs
- Cargo.toml
- src/notify.rs
🧰 Additional context used
🧬 Code graph analysis (5)
src/nixos.rs (1)
src/notify.rs (2)
ask(100-127)new(25-33)
src/home.rs (1)
src/notify.rs (2)
ask(100-127)new(25-33)
src/clean.rs (1)
src/notify.rs (2)
ask(100-127)new(25-33)
src/logging.rs (1)
src/notify.rs (3)
new(25-33)urgency(41-44)send(60-78)
src/darwin.rs (1)
src/notify.rs (2)
ask(100-127)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). (2)
- GitHub Check: Test NH on Linux
- GitHub Check: Build NH on Linux
🔇 Additional comments (7)
src/interface.rs (2)
267-275: Good addition: tri-state ask modeNotifyAskMode with Prompt/Notify/Both is clear and future‑proof. LGTM.
284-286: Arg parsing: value_enum set correctly hereOsRollbackArgs.ask correctly uses value_enum + default_missing_value. LGTM.
src/darwin.rs (1)
53-61: Nice: Display for variantReadable messages for “build”/“switch”. LGTM.
src/clean.rs (1)
523-524: LGTM: use nh_info! for deletion logsConsistent UX with the new notification‑aware logging.
src/nixos.rs (3)
8-8: LGTM: Imports are appropriate for notification support.The added imports (
warn,NotifyAskMode,nh_info,NotificationSender) are correctly structured and necessary for the notification feature.Also applies to: 18-18, 26-27
45-45: LGTM: Option presence checks are correct.The
is_some()checks appropriately detect when any ask mode is present, correctly warning users when--askhas no effect for dry-run or build-only operations.Also applies to: 281-281
425-425: LGTM: Status messages appropriately updated.The
nh_info!macro calls provide clear, user-facing status messages for rollback operations, VM instructions, and activation steps. The messaging is consistent and informative.Also applies to: 463-463, 505-505, 537-537, 562-562, 671-671, 678-678, 705-705
r0chd
force-pushed
the
notifications
branch
4 times, most recently
from
6f53a48 to
fa47570
Compare
Change-Id: I6a6a69642ac3443525df583fd200827414801d41
2 participants
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
Chores