From 0f2869e1fe4ede558d805eaf874acf6f6d7e51fa Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Thu, 7 May 2026 13:28:02 +0200 Subject: [PATCH 1/6] feat: add external-command hooks (pipe-to, exec_on_new, macros) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Users keep asking for "save to wallabag", "pipe to yt-dlp", "notify on new article", and so on. Chasing one integration at a time is a losing game. So instead, three newsboat-style primitives: * `pipe-to` pipes the focused article to a shell command's stdin (body / title / url / metadata, configurable), with the TUI suspended so pagers and editors behave. * `exec_on_new` fires a command per newly-seen item after each refresh. The first fetch of a feed is seeded silently so enabling this on a 200-item feed doesn't notify-spam you into oblivion. * Macros bind a chord (default prefix `,` then key) to a sequence of actions. Newsboat-compatible string syntax, so you can paste their configs. Templates are tokenized once via shlex and `%X` is substituted into individual argv slots — *not* run through `sh -c`. That means you can't put a `;` in your template and have a pipeline. Wrap it in a script. The alternative is letting a malicious feed title execute arbitrary commands, and that's not a trade I'm willing to make. Macros queue every step (Action / PipeTo / Exec) onto a FIFO list which the TUI loop drains in order, so newsboat-style sequential semantics are preserved across blocking steps. The terminal-suspend dance uses an RAII guard so a panicking child can't leave the user stuck in raw mode. Config and SavedData are fully back-compat — all new sections and fields are \`serde(default)\`. 103 unit tests + 4 integration tests pass; clippy clean. --- Cargo.lock | 1 + Cargo.toml | 1 + src/app.rs | 376 ++++++++++++++++++++++++++++++++- src/config.rs | 112 +++++++++- src/events.rs | 109 +++++++++- src/keybindings.rs | 503 ++++++++++++++++++++++++++++++++++++++++++--- src/tui.rs | 189 ++++++++++++++++- src/ui/modals.rs | 35 +++- 8 files changed, 1283 insertions(+), 43 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3a7f086..cc431c1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -530,6 +530,7 @@ dependencies = [ "scraper", "serde", "serde_json", + "shlex", "toml", "unicode-width 0.1.14", "url", diff --git a/Cargo.toml b/Cargo.toml index 1cf6f38..8ee8fab 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -33,6 +33,7 @@ opml = "1.1.6" toml = "0.8" scraper = "0.18" url = "2" +shlex = "1.3" [profile.release] codegen-units = 1 diff --git a/src/app.rs b/src/app.rs index ab8b8d4..7dc4713 100644 --- a/src/app.rs +++ b/src/app.rs @@ -120,6 +120,33 @@ pub struct App { pub feed_tree: Vec, pub selected_tree_item: Option, // index into feed_tree pub keybindings: crate::keybindings::KeyBindingMap, + pub macros: Vec, + pub macro_options: crate::keybindings::MacroOptions, + /// True after the macro-prefix key has been pressed and we are awaiting the next key. + pub awaiting_macro_key: bool, + /// Item IDs that exec_on_new has already fired for (persisted). + pub seen_items: HashSet, + /// Feed URLs whose first successful fetch has been observed (persisted). + pub feeds_seeded: HashSet, + /// FIFO queue of macro steps awaiting execution. Filled by the macro + /// engine in `events.rs`; drained in order by the TUI run loop after + /// each event-handling pass. Steps include Actions (which mutate app + /// state) and PipeTo/Exec (which spawn children). Draining happens at + /// the loop level because PipeTo needs the terminal handle to suspend + /// the TUI, which `handle_events` does not have access to. + pub pending_macro_steps: Vec, +} + +/// Snapshot of the focused article passed to template-expansion / pipe-payload helpers. +#[derive(Clone, Debug)] +pub struct ArticleContext<'a> { + pub title: &'a str, + pub url: Option<&'a str>, + pub author: Option<&'a str>, + pub formatted_date: Option<&'a str>, + pub feed_title: &'a str, + pub feed_url: &'a str, + pub plain_text: Option<&'a str>, } #[derive(Clone, Debug)] @@ -165,6 +192,13 @@ struct SavedData { starred_items: HashSet, #[serde(default)] last_session_time: Option, + /// Item IDs (same scheme as `read_items`) that exec_on_new has already fired for. + #[serde(default)] + seen_items: HashSet, + /// Feed URLs whose first successful fetch has been observed. Items from a + /// feed not in this set are seeded into `seen_items` silently. + #[serde(default)] + feeds_seeded: HashSet, } impl Default for App { @@ -187,6 +221,8 @@ impl App { read_items: HashSet::new(), starred_items: HashSet::new(), last_session_time: None, + seen_items: HashSet::new(), + feeds_seeded: HashSet::new(), }); // Seed bookmarks from default_feeds if no saved bookmarks exist @@ -222,6 +258,12 @@ impl App { let show_summary = last_session_time.is_some() && has_bookmarks; let (keybindings, kb_warnings) = crate::keybindings::build_keybindings(&config.keybindings); + let (macro_options, mo_warnings) = crate::keybindings::resolve_macro_options( + &config.macro_options.prefix, + &config.macro_options.pipe_default_stdin, + ); + let (macros, mac_warnings) = + crate::keybindings::build_macros(&config.macros, ¯o_options); let mut app = Self { config, @@ -275,13 +317,22 @@ impl App { feed_tree: Vec::new(), selected_tree_item: None, keybindings, + macros, + macro_options, + awaiting_macro_key: false, + seen_items: saved_data.seen_items, + feeds_seeded: saved_data.feeds_seeded, + pending_macro_steps: Vec::new(), }; app.update_dashboard(); app.rebuild_feed_tree(); - if !kb_warnings.is_empty() { - app.error = Some(format!("Keybinding config: {}", kb_warnings.join("; "))); + let mut all_warnings = kb_warnings; + all_warnings.extend(mo_warnings); + all_warnings.extend(mac_warnings); + if !all_warnings.is_empty() { + app.error = Some(format!("Config: {}", all_warnings.join("; "))); } app @@ -354,6 +405,8 @@ impl App { read_items: HashSet::new(), starred_items: HashSet::new(), last_session_time: None, + seen_items: HashSet::new(), + feeds_seeded: HashSet::new(), }); } @@ -374,6 +427,8 @@ impl App { read_items: self.read_items.clone(), starred_items: self.starred_items.clone(), last_session_time: Some(Utc::now().to_rfc3339()), + seen_items: self.seen_items.clone(), + feeds_seeded: self.feeds_seeded.clone(), }; let json = serde_json::to_string(&saved_data)?; @@ -598,10 +653,7 @@ impl App { pub(crate) fn get_item_id(&self, feed_idx: usize, item_idx: usize) -> String { if let Some(feed) = self.feeds.get(feed_idx) { if let Some(item) = feed.items.get(item_idx) { - if let Some(link) = &item.link { - return link.clone(); - } - return format!("{}_{}", feed.url, item.title); + return make_item_id(feed, item); } } String::new() @@ -1549,6 +1601,166 @@ impl App { self.error = Some("No links or images found in this article".to_string()); } } + + /// Resolve the focused article based on the current view + selection. + /// Returns None if nothing is selected or the indices don't resolve. + pub fn current_article_context(&self) -> Option> { + let (feed_idx, item_idx) = match self.view { + View::Dashboard => { + let sel = self.selected_item?; + let active = self.active_dashboard_items(); + if sel >= active.len() { + return None; + } + active[sel] + } + View::FeedItems | View::FeedItemDetail => { + let feed_idx = self.selected_feed?; + let item_idx = self.selected_item?; + (feed_idx, item_idx) + } + View::Starred => { + let sel = self.selected_item?; + let starred = self.get_starred_dashboard_items(); + if sel >= starred.len() { + return None; + } + starred[sel] + } + _ => return None, + }; + let feed = self.feeds.get(feed_idx)?; + let item = feed.items.get(item_idx)?; + Some(ArticleContext { + title: &item.title, + url: item.link.as_deref(), + author: item.author.as_deref(), + formatted_date: item.formatted_date.as_deref(), + feed_title: &feed.title, + feed_url: &feed.url, + plain_text: item.plain_text.as_deref(), + }) + } + + /// Look up a macro whose trigger matches the given key event. + pub fn macro_for_key( + &self, + key: &crossterm::event::KeyEvent, + ) -> Option<&crate::keybindings::MacroBinding> { + self.macros.iter().find(|m| m.trigger.matches(key)) + } +} + +/// Build the same identifier scheme used by `read_items`/`starred_items` from +/// a feed + item that may not yet be in `App::feeds`. +pub fn make_item_id(feed: &Feed, item: &FeedItem) -> String { + if let Some(link) = &item.link { + return link.clone(); + } + format!("{}_{}", feed.url, item.title) +} + +/// Update `seen_items` and `feeds_seeded` to reflect this feed having been +/// successfully fetched. Returns the indices into `feed.items` of items +/// that are newly seen *and should fire exec_on_new*. On the first +/// successful fetch of a feed (URL not yet in `feeds_seeded`), the seen +/// set is populated silently and an empty Vec is returned. +pub fn mark_feed_seen(app: &mut App, feed: &Feed) -> Vec { + let is_first_fetch = !app.feeds_seeded.contains(&feed.url); + let mut newly_seen = Vec::new(); + for (idx, item) in feed.items.iter().enumerate() { + let id = make_item_id(feed, item); + let inserted = app.seen_items.insert(id); + if inserted && !is_first_fetch { + newly_seen.push(idx); + } + } + if is_first_fetch { + app.feeds_seeded.insert(feed.url.clone()); + } + newly_seen +} + +/// Substitute newsboat-style %X placeholders into each argv token. Untrusted +/// content is never interpreted by a shell — placeholders are inserted +/// verbatim into individual argv elements. +/// +/// Variables: %t title, %u url, %a author, %d formatted-date, +/// %f feed-title, %F feed-url, %% literal %. +pub fn expand_argv_template(template: &[String], ctx: &ArticleContext<'_>) -> Vec { + template + .iter() + .map(|tok| expand_one_token(tok, ctx)) + .collect() +} + +fn expand_one_token(tok: &str, ctx: &ArticleContext<'_>) -> String { + let mut out = String::with_capacity(tok.len()); + let mut chars = tok.chars().peekable(); + while let Some(c) = chars.next() { + if c != '%' { + out.push(c); + continue; + } + match chars.next() { + Some('%') => out.push('%'), + Some('t') => out.push_str(ctx.title), + Some('u') => out.push_str(ctx.url.unwrap_or("")), + Some('a') => out.push_str(ctx.author.unwrap_or("")), + Some('d') => out.push_str(ctx.formatted_date.unwrap_or("")), + Some('f') => out.push_str(ctx.feed_title), + Some('F') => out.push_str(ctx.feed_url), + Some(other) => { + out.push('%'); + out.push(other); + } + None => out.push('%'), + } + } + out +} + +/// Build the byte payload that gets piped to a `pipe-to` child's stdin. +pub fn make_pipe_payload(ctx: &ArticleContext<'_>, kind: crate::keybindings::StdinKind) -> Vec { + use crate::keybindings::StdinKind; + match kind { + StdinKind::None => Vec::new(), + StdinKind::Title => ctx.title.as_bytes().to_vec(), + StdinKind::Url => ctx.url.unwrap_or("").as_bytes().to_vec(), + StdinKind::Body => ctx + .plain_text + .map(|s| s.as_bytes().to_vec()) + .unwrap_or_default(), + StdinKind::Metadata => { + let mut out = String::new(); + out.push_str("Title: "); + out.push_str(ctx.title); + out.push('\n'); + if let Some(u) = ctx.url { + out.push_str("URL: "); + out.push_str(u); + out.push('\n'); + } + if let Some(a) = ctx.author { + out.push_str("Author: "); + out.push_str(a); + out.push('\n'); + } + if let Some(d) = ctx.formatted_date { + out.push_str("Date: "); + out.push_str(d); + out.push('\n'); + } + out.push_str("Feed: "); + out.push_str(ctx.feed_title); + out.push('\n'); + out.push('\n'); + if let Some(body) = ctx.plain_text { + out.push_str(body); + } + out.into_bytes() + } + } } #[cfg(test)] @@ -2045,4 +2257,156 @@ mod tests { assert_eq!(app.extracted_links.len(), 1); assert_eq!(app.extracted_links[0].url, "https://example.com/about"); } + + fn synthetic_ctx() -> ArticleContext<'static> { + ArticleContext { + title: "Hello %u world", + url: Some("https://ex.com/a"), + author: Some("Alice"), + formatted_date: Some("2 hours ago"), + feed_title: "Example", + feed_url: "https://ex.com/feed.xml", + plain_text: Some("body text"), + } + } + + #[test] + fn test_expand_argv_template_per_token() { + let tmpl = vec![ + "yt-dlp".to_string(), + "%u".to_string(), + "--title=%t".to_string(), + ]; + let argv = expand_argv_template(&tmpl, &synthetic_ctx()); + assert_eq!(argv[0], "yt-dlp"); + // The url replaces %u literally — but the title contains a %u which + // must NOT be re-expanded (we only expand once, in the template, not + // in the substituted content). + assert_eq!(argv[1], "https://ex.com/a"); + assert_eq!(argv[2], "--title=Hello %u world"); + } + + #[test] + fn test_expand_argv_template_percent_literal_and_unknown() { + let tmpl = vec!["a%%b".to_string(), "x%zy".to_string()]; + let argv = expand_argv_template(&tmpl, &synthetic_ctx()); + assert_eq!(argv[0], "a%b"); + // Unknown escapes are passed through verbatim. + assert_eq!(argv[1], "x%zy"); + } + + #[test] + fn test_make_pipe_payload_kinds() { + use crate::keybindings::StdinKind; + let ctx = synthetic_ctx(); + assert_eq!(make_pipe_payload(&ctx, StdinKind::None), Vec::::new()); + assert_eq!(make_pipe_payload(&ctx, StdinKind::Title), b"Hello %u world"); + assert_eq!(make_pipe_payload(&ctx, StdinKind::Url), b"https://ex.com/a"); + assert_eq!(make_pipe_payload(&ctx, StdinKind::Body), b"body text"); + let metadata = make_pipe_payload(&ctx, StdinKind::Metadata); + let s = String::from_utf8(metadata).unwrap(); + assert!(s.contains("Title: Hello %u world")); + assert!(s.contains("URL: https://ex.com/a")); + assert!(s.contains("Author: Alice")); + assert!(s.contains("Feed: Example")); + assert!(s.ends_with("body text")); + } + + fn synthetic_feed(url: &str, item_titles: &[&str]) -> Feed { + let items = item_titles + .iter() + .map(|t| FeedItem { + title: t.to_string(), + link: Some(format!("https://ex.com/a/{}", t)), + description: None, + pub_date: None, + author: None, + formatted_date: None, + parsed_date: None, + plain_text: None, + title_lower: t.to_lowercase(), + }) + .collect(); + Feed { + url: url.to_string(), + title: "Example".to_string(), + items, + title_lower: "example".to_string(), + } + } + + #[test] + fn test_mark_feed_seen_first_fetch_seeds_silently() { + let mut app = App::new(); + let feed = synthetic_feed("https://ex.com/feed.xml", &["A", "B", "C"]); + + let newly = mark_feed_seen(&mut app, &feed); + assert!( + newly.is_empty(), + "first fetch must return no newly-seen items" + ); + assert_eq!(app.seen_items.len(), 3); + assert!(app.feeds_seeded.contains("https://ex.com/feed.xml")); + } + + #[test] + fn test_mark_feed_seen_second_fetch_returns_only_new() { + let mut app = App::new(); + let feed_v1 = synthetic_feed("https://ex.com/feed.xml", &["A", "B"]); + mark_feed_seen(&mut app, &feed_v1); + + let feed_v2 = synthetic_feed("https://ex.com/feed.xml", &["A", "B", "C", "D"]); + let newly = mark_feed_seen(&mut app, &feed_v2); + // Indices 2 and 3 (C, D) are new; A and B are already seen. + assert_eq!(newly, vec![2, 3]); + assert_eq!(app.seen_items.len(), 4); + } + + #[test] + fn test_mark_feed_seen_no_changes_returns_empty() { + let mut app = App::new(); + let feed = synthetic_feed("https://ex.com/feed.xml", &["A", "B"]); + mark_feed_seen(&mut app, &feed); + let newly = mark_feed_seen(&mut app, &feed); + assert!(newly.is_empty()); + } + + #[test] + fn test_mark_feed_seen_distinct_feeds_independent() { + let mut app = App::new(); + let feed_a = synthetic_feed("https://ex.com/a.xml", &["A1"]); + let feed_b = synthetic_feed("https://ex.com/b.xml", &["B1"]); + // First fetches both seed silently. + assert!(mark_feed_seen(&mut app, &feed_a).is_empty()); + assert!(mark_feed_seen(&mut app, &feed_b).is_empty()); + assert!(app.feeds_seeded.contains("https://ex.com/a.xml")); + assert!(app.feeds_seeded.contains("https://ex.com/b.xml")); + } + + #[test] + fn test_make_item_id_prefers_link() { + let mut feed = Feed { + url: "https://ex.com/feed.xml".to_string(), + title: "Example".to_string(), + items: vec![], + title_lower: "example".to_string(), + }; + let item = FeedItem { + title: "T".to_string(), + link: Some("https://ex.com/a".to_string()), + description: None, + pub_date: None, + author: None, + formatted_date: None, + parsed_date: None, + plain_text: None, + title_lower: "t".to_string(), + }; + feed.items.push(item.clone()); + assert_eq!(make_item_id(&feed, &item), "https://ex.com/a"); + let mut item2 = item.clone(); + item2.link = None; + item2.title = "T2".to_string(); + assert_eq!(make_item_id(&feed, &item2), "https://ex.com/feed.xml_T2"); + } } diff --git a/src/config.rs b/src/config.rs index 1e8259d..1483e28 100644 --- a/src/config.rs +++ b/src/config.rs @@ -18,6 +18,46 @@ pub struct Config { pub default_feeds: Vec, #[serde(default, skip_serializing_if = "HashMap::is_empty")] pub keybindings: HashMap, + #[serde(default)] + pub hooks: HooksConfig, + #[serde(default, skip_serializing_if = "HashMap::is_empty")] + pub macros: HashMap, + #[serde(default)] + pub macro_options: MacroOptionsConfig, +} + +#[derive(Clone, Debug, Default, Serialize, Deserialize)] +pub struct HooksConfig { + /// Shell-tokenized command template fired once per newly-seen item after a refresh. + /// Variables: %t title, %u url, %a author, %d formatted-date, %f feed-title, %F feed-url. + /// First-ever fetch of a feed is seeded silently (no firehose). + #[serde(default, skip_serializing_if = "Option::is_none")] + pub exec_on_new: Option, +} + +#[derive(Clone, Debug, Serialize, Deserialize)] +pub struct MacroOptionsConfig { + #[serde(default = "default_macro_prefix")] + pub prefix: String, + #[serde(default = "default_pipe_stdin")] + pub pipe_default_stdin: String, +} + +fn default_macro_prefix() -> String { + ",".to_string() +} + +fn default_pipe_stdin() -> String { + "body".to_string() +} + +impl Default for MacroOptionsConfig { + fn default() -> Self { + Self { + prefix: default_macro_prefix(), + pipe_default_stdin: default_pipe_stdin(), + } + } } #[derive(Clone, Debug, Serialize, Deserialize)] @@ -364,7 +404,36 @@ impl Config { # [[default_feeds]]\n\ # url = \"https://private.example.com/feed.xml\"\n\ # [default_feeds.headers]\n\ - # Authorization = \"Bearer your_token_here\"\n", + # Authorization = \"Bearer your_token_here\"\n\ + #\n\ + # ── External-command hooks ──────────────────────────────\n\ + #\n\ + # Macros bind a key (invoked as , default prefix is ',') to\n\ + # an ordered chain of actions. Steps are separated by ';'. Recognized\n\ + # action names mirror the existing keybinding actions (use dashes), plus\n\ + # the synthetic ops `pipe-to` and `exec`. Variables expanded per token:\n\ + # %t title %u url %a author %d date %f feed-title %F feed-url\n\ + #\n\ + # IMPORTANT: command templates are NOT run through a shell. They are\n\ + # tokenized once via shell-style splitting and %X is substituted into\n\ + # individual argv tokens. If you need pipes or redirection, write a\n\ + # shell script and invoke that.\n\ + #\n\ + # [macros]\n\ + # y = 'open-in-browser ; pipe-to \"yt-dlp %u\"'\n\ + # w = 'pipe-to \"wallabag-cli add %u\" -- \"Save to Wallabag\"'\n\ + # n = 'pipe-to \"tee out.txt\" stdin=metadata'\n\ + #\n\ + # [macro_options]\n\ + # prefix = \",\" # the macro-prefix key\n\ + # pipe_default_stdin = \"body\" # body | title | url | metadata | none\n\ + #\n\ + # The exec_on_new hook fires once per newly-seen item on each refresh.\n\ + # The first successful fetch of a feed seeds the seen set silently to\n\ + # avoid a firehose on initial load. Children are spawned detached.\n\ + #\n\ + # [hooks]\n\ + # exec_on_new = 'notify-send \"New: %t\" \"%f\"'\n", toml ) } @@ -403,6 +472,47 @@ mod tests { assert_eq!(headers.get("X-Custom").unwrap(), "value"); } + #[test] + fn test_hooks_and_macros_round_trip() { + let toml_str = r#" + [hooks] + exec_on_new = 'notify-send "New: %t" "%f"' + + [macros] + y = 'open-in-browser ; pipe-to "yt-dlp %u"' + + [macro_options] + prefix = "," + pipe_default_stdin = "metadata" + "#; + let config: Config = toml::from_str(toml_str).unwrap(); + assert_eq!( + config.hooks.exec_on_new.as_deref(), + Some(r#"notify-send "New: %t" "%f""#) + ); + assert_eq!( + config.macros.get("y").map(String::as_str), + Some(r#"open-in-browser ; pipe-to "yt-dlp %u""#) + ); + assert_eq!(config.macro_options.prefix, ","); + assert_eq!(config.macro_options.pipe_default_stdin, "metadata"); + } + + #[test] + fn test_config_back_compat_without_new_sections() { + // An old config file with no [hooks]/[macros]/[macro_options] must still load. + let toml_str = r#" + [general] + max_dashboard_items = 50 + "#; + let config: Config = toml::from_str(toml_str).unwrap(); + assert_eq!(config.general.max_dashboard_items, 50); + assert!(config.hooks.exec_on_new.is_none()); + assert!(config.macros.is_empty()); + assert_eq!(config.macro_options.prefix, ","); + assert_eq!(config.macro_options.pipe_default_stdin, "body"); + } + #[test] fn test_config_serialization() { let config = Config::default(); diff --git a/src/events.rs b/src/events.rs index ededb91..1bbcf29 100644 --- a/src/events.rs +++ b/src/events.rs @@ -14,8 +14,11 @@ // - All text input modes (InsertUrl, SearchMode, CategoryNameInput) // - Detail view: g/G and Ctrl+u/Ctrl+d for scrolling -use crate::app::{AddFeedResult, App, CategoryAction, InputMode, TimeFilter, TreeItem, View}; -use crate::keybindings::KeyAction; +use crate::app::{ + expand_argv_template, make_pipe_payload, AddFeedResult, App, CategoryAction, InputMode, + TimeFilter, TreeItem, View, +}; +use crate::keybindings::{KeyAction, MacroBinding, StdinKind}; use anyhow::Result; use crossterm::event::{ self, Event, KeyCode, KeyEventKind, KeyModifiers, MouseButton, MouseEvent, MouseEventKind, @@ -89,6 +92,82 @@ fn handle_toggle_read_current(app: &mut App) { } } +// ── Macro engine ─────────────────────────────────────────────────── +// +// Macros are checked at the top of `handle_key_event` and bypass the +// per-view dispatch entirely. ALL steps (Action / PipeTo / Exec) are +// queued onto `app.pending_macro_steps` so they run in newsboat-style +// sequential order. The TUI loop drains the queue after each +// event-handling pass — actions mutate state inline, PipeTo suspends +// the terminal, Exec spawns detached. + +/// Queue every step of a macro for the TUI drain in order. +fn run_macro(app: &mut App, mac: &MacroBinding) { + for step in &mac.steps { + app.pending_macro_steps.push(step.clone()); + } +} + +/// Build the (argv, stdin payload) tuple for a queued PipeTo step using +/// the current article context. Returns None if no article is focused. +pub(crate) fn build_pipe_invocation( + app: &App, + argv_template: &[String], + stdin: StdinKind, +) -> Option<(Vec, Vec)> { + let ctx = app.current_article_context()?; + let argv = expand_argv_template(argv_template, &ctx); + let payload = make_pipe_payload(&ctx, stdin); + Some((argv, payload)) +} + +/// Build the argv for a queued Exec step using the current article context. +/// Returns None if no article is focused. +pub(crate) fn build_exec_invocation(app: &App, argv_template: &[String]) -> Option> { + let ctx = app.current_article_context()?; + Some(expand_argv_template(argv_template, &ctx)) +} + +/// Run a `KeyAction` outside of normal key dispatch (called from the +/// macro queue drain). Only a curated subset is supported — actions whose +/// meaning is well-defined without view-specific input mode context. +pub(crate) fn dispatch_action(app: &mut App, action: KeyAction) { + match action { + KeyAction::Refresh => handle_refresh(app), + KeyAction::ToggleTheme => handle_toggle_theme(app), + KeyAction::Help => handle_show_help(app), + KeyAction::ToggleStar => handle_toggle_star_current(app), + KeyAction::ToggleRead => handle_toggle_read_current(app), + KeyAction::OpenInBrowser => match app.current_article_context() { + Some(ctx) => { + if let Some(url) = ctx.url { + if let Err(e) = open::that(url) { + app.error = Some(format!("Failed to open link: {}", e)); + } + } else { + app.error = Some("No URL on this article".to_string()); + } + } + None => app.error = Some("open-in-browser: no article in focus".to_string()), + }, + KeyAction::MarkAllRead => match app.mark_all_dashboard_read() { + Ok(count) => { + app.success_message = Some(format!("\u{2713} Marked {} items as read", count)); + app.success_message_time = Some(std::time::Instant::now()); + app.apply_filters(); + } + Err(e) => app.error = Some(format!("Failed to mark all read: {}", e)), + }, + KeyAction::ExtractLinks => app.extract_links_from_current_item(), + other => { + app.error = Some(format!( + "macro: action '{:?}' not supported in macros", + other + )); + } + } +} + // ── Event entry point ────────────────────────────────────────────── pub(crate) fn handle_events(app: &mut App) -> Result { @@ -164,6 +243,32 @@ pub(crate) fn handle_key_event(app: &mut App, key: crossterm::event::KeyEvent) - if app.key_matches(KeyAction::ForceQuit, &key) { return Ok(true); } + + // Macro engine: handle the macro-prefix follow-up key, then check whether + // this key IS the macro prefix. Only active in Normal input mode so it + // never interferes with text-input or filter-cycling modes. + if app.input_mode == InputMode::Normal { + if app.awaiting_macro_key { + app.awaiting_macro_key = false; + match app.macro_for_key(&key) { + Some(mac) => { + let mac = mac.clone(); + run_macro(app, &mac); + } + None => { + app.error = Some("No macro bound to that key".to_string()); + } + } + return Ok(false); + } + if app.macro_options.prefix.matches(&key) && !app.macros.is_empty() { + app.awaiting_macro_key = true; + app.success_message = Some("Macro prefix... press macro key".to_string()); + app.success_message_time = Some(std::time::Instant::now()); + return Ok(false); + } + } + match app.input_mode { InputMode::Normal => match app.view { View::Dashboard => match key.code { diff --git a/src/keybindings.rs b/src/keybindings.rs index d360b67..c4128c6 100644 --- a/src/keybindings.rs +++ b/src/keybindings.rs @@ -2,6 +2,69 @@ use crossterm::event::{KeyCode, KeyEvent, KeyModifiers}; use std::collections::HashMap; use std::str::FromStr; +/// What gets piped to stdin when a `pipe-to` macro step runs. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub enum StdinKind { + None, + #[default] + Body, + Title, + Url, + Metadata, +} + +impl FromStr for StdinKind { + type Err = (); + fn from_str(s: &str) -> Result { + match s.to_ascii_lowercase().as_str() { + "none" => Ok(Self::None), + "body" => Ok(Self::Body), + "title" => Ok(Self::Title), + "url" => Ok(Self::Url), + "metadata" => Ok(Self::Metadata), + _ => Err(()), + } + } +} + +/// A single step inside a macro chain. +#[derive(Clone, Debug)] +pub enum MacroStep { + /// Invoke an existing keybinding action (e.g. open-in-browser, toggle-star). + Action(KeyAction), + /// Run argv with article content piped to stdin. + PipeTo { + argv_template: Vec, + stdin: StdinKind, + }, + /// Run argv with no stdin. + Exec { argv_template: Vec }, +} + +/// A macro: a key trigger + an ordered sequence of steps + optional description. +#[derive(Clone, Debug)] +pub struct MacroBinding { + pub trigger: KeyBinding, + pub steps: Vec, + pub description: Option, +} + +/// Top-level options for the macro engine. +#[derive(Clone, Debug)] +pub struct MacroOptions { + pub prefix: KeyBinding, + pub pipe_default_stdin: StdinKind, +} + +impl Default for MacroOptions { + fn default() -> Self { + Self { + prefix: KeyBinding::new(KeyCode::Char(',')), + pipe_default_stdin: StdinKind::Body, + } + } +} + #[derive(Clone, Debug, Hash, Eq, PartialEq)] pub enum KeyAction { // Global @@ -387,43 +450,282 @@ pub fn build_keybindings( (map, warnings) } -/// Get display string for the first binding of an action -pub fn key_display(action: &KeyAction, map: &KeyBindingMap) -> String { - if let Some(bindings) = map.get(action) { - if let Some(binding) = bindings.first() { - let mut parts = Vec::new(); - if binding.modifiers.contains(KeyModifiers::CONTROL) { - parts.push("Ctrl".to_string()); +// ── Macro parsing ───────────────────────────────────────────────── +// +// Newsboat-compatible string syntax: +// ; [args]; ... [-- ""] +// +// Examples: +// open-in-browser ; pipe-to "yt-dlp %u" +// pipe-to "wallabag-cli add %u" -- "Save to Wallabag" +// pipe-to "tee out.txt" stdin=metadata +// +// Recognized op names mirror existing KeyAction names but use dashes +// (newsboat convention), e.g. `open-in-browser` <-> `open_in_browser`. +// Two synthetic ops, `pipe-to` and `exec`, take a quoted command template +// followed by optional `key=value` modifiers (currently only `stdin=`). + +/// Split a string on a single ASCII separator, ignoring occurrences inside +/// double-quoted spans. Backslash escapes the next character. +fn split_top_level(s: &str, sep: u8) -> Vec { + let bytes = s.as_bytes(); + let mut out = Vec::new(); + let mut start = 0; + let mut in_quote = false; + let mut escape = false; + let mut i = 0; + while i < bytes.len() { + let b = bytes[i]; + if escape { + escape = false; + i += 1; + continue; + } + if b == b'\\' { + escape = true; + i += 1; + continue; + } + if b == b'"' { + in_quote = !in_quote; + i += 1; + continue; + } + if !in_quote && b == sep { + out.push(s[start..i].to_string()); + start = i + 1; + } + i += 1; + } + out.push(s[start..].to_string()); + out +} + +/// Detect a trailing ` -- "description"` outside double-quoted spans. +fn split_description(s: &str) -> (&str, Option) { + let bytes = s.as_bytes(); + let mut in_quote = false; + let mut escape = false; + let mut i = 0; + while i < bytes.len() { + let b = bytes[i]; + if escape { + escape = false; + i += 1; + continue; + } + if b == b'\\' { + escape = true; + i += 1; + continue; + } + if b == b'"' { + in_quote = !in_quote; + i += 1; + continue; + } + if !in_quote + && b == b' ' + && i + 3 < bytes.len() + && bytes[i + 1] == b'-' + && bytes[i + 2] == b'-' + && bytes[i + 3] == b' ' + { + // i and i+4 are ASCII boundaries; safe to slice. + let body = &s[..i]; + let desc_part = s[i + 4..].trim(); + let unquoted = desc_part + .strip_prefix('"') + .and_then(|d| d.strip_suffix('"')) + .unwrap_or(desc_part); + let desc = unquoted.to_string(); + return (body, if desc.is_empty() { None } else { Some(desc) }); + } + i += 1; + } + (s, None) +} + +fn split_op_and_rest(step: &str) -> Option<(&str, &str)> { + let s = step.trim(); + if s.is_empty() { + return None; + } + match s.find(char::is_whitespace) { + Some(idx) => Some((&s[..idx], s[idx..].trim_start())), + None => Some((s, "")), + } +} + +fn parse_pipe_or_exec_args(rest: &str) -> Result<(Vec, Vec), String> { + // shlex::split returns None on unbalanced quotes + let tokens = shlex::split(rest).ok_or_else(|| format!("could not tokenize: {}", rest))?; + if tokens.is_empty() { + return Err("missing command argument".into()); + } + let argv = shlex::split(&tokens[0]) + .ok_or_else(|| format!("could not tokenize command: {}", tokens[0]))?; + if argv.is_empty() { + return Err("command is empty".into()); + } + let modifiers = tokens[1..].to_vec(); + Ok((argv, modifiers)) +} + +/// Parse one macro definition string into a sequence of steps and +/// an optional human description. +pub fn parse_macro_string( + raw: &str, + default_stdin: StdinKind, +) -> Result<(Vec, Option), String> { + let (body, description) = split_description(raw); + let parts = split_top_level(body, b';'); + let mut steps = Vec::new(); + for part in parts { + let part = part.trim(); + if part.is_empty() { + continue; + } + let (op, rest) = split_op_and_rest(part).ok_or_else(|| "empty step".to_string())?; + match op { + "pipe-to" | "pipe_to" => { + let (argv_template, modifiers) = + parse_pipe_or_exec_args(rest).map_err(|e| format!("pipe-to: {}", e))?; + let mut stdin = default_stdin; + for m in modifiers { + let (k, v) = m + .split_once('=') + .ok_or_else(|| format!("pipe-to: unknown modifier '{}'", m))?; + match k { + "stdin" => { + stdin = StdinKind::from_str(v) + .map_err(|_| format!("pipe-to: invalid stdin kind '{}'", v))?; + } + _ => return Err(format!("pipe-to: unknown modifier '{}'", k)), + } + } + steps.push(MacroStep::PipeTo { + argv_template, + stdin, + }); } - if binding.modifiers.contains(KeyModifiers::SHIFT) { - parts.push("Shift".to_string()); + "exec" => { + let (argv_template, modifiers) = + parse_pipe_or_exec_args(rest).map_err(|e| format!("exec: {}", e))?; + if !modifiers.is_empty() { + return Err(format!("exec takes no modifiers, got: {:?}", modifiers)); + } + steps.push(MacroStep::Exec { argv_template }); } - if binding.modifiers.contains(KeyModifiers::ALT) { - parts.push("Alt".to_string()); + _ => { + let normalized = op.replace('-', "_"); + let action = KeyAction::from_str(&normalized) + .map_err(|_| format!("unknown action '{}'", op))?; + if !rest.is_empty() { + return Err(format!("action '{}' takes no arguments, got: {}", op, rest)); + } + steps.push(MacroStep::Action(action)); } - let key_name = match binding.code { - KeyCode::Char(' ') => "Space".to_string(), - KeyCode::Char(c) => c.to_string(), - KeyCode::Enter => "Enter".to_string(), - KeyCode::Esc => "Esc".to_string(), - KeyCode::Tab => "Tab".to_string(), - KeyCode::Backspace => "Backspace".to_string(), - KeyCode::Up => "\u{2191}".to_string(), - KeyCode::Down => "\u{2193}".to_string(), - KeyCode::Left => "\u{2190}".to_string(), - KeyCode::Right => "\u{2192}".to_string(), - KeyCode::Home => "Home".to_string(), - KeyCode::End => "End".to_string(), - KeyCode::PageUp => "PgUp".to_string(), - KeyCode::PageDown => "PgDn".to_string(), - KeyCode::F(n) => format!("F{}", n), - _ => "?".to_string(), - }; - parts.push(key_name); - return parts.join("+"); } } - "?".to_string() + if steps.is_empty() { + return Err("macro has no steps".into()); + } + Ok((steps, description)) +} + +/// Build the parsed macro list from a config map of `key -> definition string`. +/// Returns the bindings and a list of warnings for invalid entries. +pub fn build_macros( + config_macros: &HashMap, + options: &MacroOptions, +) -> (Vec, Vec) { + let mut out = Vec::new(); + let mut warnings = Vec::new(); + let mut keys: Vec<&String> = config_macros.keys().collect(); + keys.sort(); + for key in keys { + let body = &config_macros[key]; + let trigger = match parse_key_string(key) { + Some(b) => b, + None => { + warnings.push(format!("could not parse macro key '{}'", key)); + continue; + } + }; + match parse_macro_string(body, options.pipe_default_stdin) { + Ok((steps, description)) => out.push(MacroBinding { + trigger, + steps, + description, + }), + Err(e) => warnings.push(format!("macro '{}': {}", key, e)), + } + } + (out, warnings) +} + +/// Resolve `MacroOptions` from raw string-typed config values. +pub fn resolve_macro_options( + prefix_str: &str, + pipe_default_stdin_str: &str, +) -> (MacroOptions, Vec) { + let mut options = MacroOptions::default(); + let mut warnings = Vec::new(); + match parse_key_string(prefix_str) { + Some(b) => options.prefix = b, + None => warnings.push(format!("invalid macro prefix '{}'", prefix_str)), + } + match StdinKind::from_str(pipe_default_stdin_str) { + Ok(k) => options.pipe_default_stdin = k, + Err(_) => warnings.push(format!( + "invalid pipe_default_stdin '{}'", + pipe_default_stdin_str + )), + } + (options, warnings) +} + +/// Format a single `KeyBinding` like `Ctrl+q`, `Shift+Tab`, `,`, `Space`. +pub fn binding_display(binding: &KeyBinding) -> String { + let mut parts = Vec::new(); + if binding.modifiers.contains(KeyModifiers::CONTROL) { + parts.push("Ctrl".to_string()); + } + if binding.modifiers.contains(KeyModifiers::SHIFT) { + parts.push("Shift".to_string()); + } + if binding.modifiers.contains(KeyModifiers::ALT) { + parts.push("Alt".to_string()); + } + let key_name = match binding.code { + KeyCode::Char(' ') => "Space".to_string(), + KeyCode::Char(c) => c.to_string(), + KeyCode::Enter => "Enter".to_string(), + KeyCode::Esc => "Esc".to_string(), + KeyCode::Tab => "Tab".to_string(), + KeyCode::Backspace => "Backspace".to_string(), + KeyCode::Up => "\u{2191}".to_string(), + KeyCode::Down => "\u{2193}".to_string(), + KeyCode::Left => "\u{2190}".to_string(), + KeyCode::Right => "\u{2192}".to_string(), + KeyCode::Home => "Home".to_string(), + KeyCode::End => "End".to_string(), + KeyCode::PageUp => "PgUp".to_string(), + KeyCode::PageDown => "PgDn".to_string(), + KeyCode::F(n) => format!("F{}", n), + _ => "?".to_string(), + }; + parts.push(key_name); + parts.join("+") +} + +/// Get display string for the first binding of an action +pub fn key_display(action: &KeyAction, map: &KeyBindingMap) -> String { + map.get(action) + .and_then(|bindings| bindings.first()) + .map(binding_display) + .unwrap_or_else(|| "?".to_string()) } #[cfg(test)] @@ -604,4 +906,141 @@ mod tests { // Multi-char key name that isn't a special key assert!(parse_key_string("abc").is_none()); } + + // ── Macro parser ────────────────────────────────────────────── + + #[test] + fn test_macro_single_action() { + let (steps, desc) = parse_macro_string("open-in-browser", StdinKind::Body).unwrap(); + assert_eq!(steps.len(), 1); + assert!(matches!( + steps[0], + MacroStep::Action(KeyAction::OpenInBrowser) + )); + assert!(desc.is_none()); + } + + #[test] + fn test_macro_chain_with_pipe() { + let (steps, desc) = + parse_macro_string(r#"open-in-browser ; pipe-to "yt-dlp %u""#, StdinKind::Body) + .unwrap(); + assert_eq!(steps.len(), 2); + assert!(matches!( + steps[0], + MacroStep::Action(KeyAction::OpenInBrowser) + )); + match &steps[1] { + MacroStep::PipeTo { + argv_template, + stdin, + } => { + assert_eq!(argv_template, &vec!["yt-dlp".to_string(), "%u".to_string()]); + assert_eq!(*stdin, StdinKind::Body); + } + other => panic!("expected PipeTo, got {:?}", other), + } + assert!(desc.is_none()); + } + + #[test] + fn test_macro_with_description() { + let (steps, desc) = + parse_macro_string(r#"toggle-star -- "Star current item""#, StdinKind::Body).unwrap(); + assert_eq!(steps.len(), 1); + assert_eq!(desc, Some("Star current item".to_string())); + } + + #[test] + fn test_macro_pipe_with_stdin_modifier() { + let (steps, _) = + parse_macro_string(r#"pipe-to "tee out.txt" stdin=url"#, StdinKind::Body).unwrap(); + match &steps[0] { + MacroStep::PipeTo { + argv_template, + stdin, + } => { + assert_eq!( + argv_template, + &vec!["tee".to_string(), "out.txt".to_string()] + ); + assert_eq!(*stdin, StdinKind::Url); + } + other => panic!("expected PipeTo, got {:?}", other), + } + } + + #[test] + fn test_macro_exec_no_stdin() { + let (steps, _) = + parse_macro_string(r#"exec "notify-send hello""#, StdinKind::Body).unwrap(); + match &steps[0] { + MacroStep::Exec { argv_template } => { + assert_eq!( + argv_template, + &vec!["notify-send".to_string(), "hello".to_string()] + ); + } + other => panic!("expected Exec, got {:?}", other), + } + } + + #[test] + fn test_macro_unknown_action_errors() { + assert!(parse_macro_string("frobnicate", StdinKind::Body).is_err()); + } + + #[test] + fn test_macro_unbalanced_quote_errors() { + assert!(parse_macro_string(r#"pipe-to "tee out.txt"#, StdinKind::Body).is_err()); + } + + #[test] + fn test_macro_empty_errors() { + assert!(parse_macro_string("", StdinKind::Body).is_err()); + assert!(parse_macro_string(" ; ; ", StdinKind::Body).is_err()); + } + + #[test] + fn test_macro_semicolon_inside_quotes_not_split() { + let (steps, _) = parse_macro_string(r#"pipe-to "echo a;b""#, StdinKind::Body).unwrap(); + assert_eq!(steps.len(), 1); + match &steps[0] { + MacroStep::PipeTo { argv_template, .. } => { + // shlex splits "echo a;b" into ["echo", "a;b"] + assert_eq!(argv_template, &vec!["echo".to_string(), "a;b".to_string()]); + } + _ => panic!(), + } + } + + #[test] + fn test_build_macros_warns_on_bad_key() { + let mut macros = HashMap::new(); + macros.insert("notakey++".to_string(), "open-in-browser".to_string()); + let (out, warnings) = build_macros(¯os, &MacroOptions::default()); + assert!(out.is_empty()); + assert_eq!(warnings.len(), 1); + assert!(warnings[0].contains("could not parse macro key")); + } + + #[test] + fn test_build_macros_warns_on_bad_body() { + let mut macros = HashMap::new(); + macros.insert("y".to_string(), "frobnicate".to_string()); + let (out, warnings) = build_macros(¯os, &MacroOptions::default()); + assert!(out.is_empty()); + assert_eq!(warnings.len(), 1); + assert!(warnings[0].contains("unknown action")); + } + + #[test] + fn test_resolve_macro_options() { + let (opts, warnings) = resolve_macro_options(",", "metadata"); + assert!(warnings.is_empty()); + assert_eq!(opts.pipe_default_stdin, StdinKind::Metadata); + + let (_, warnings) = resolve_macro_options("notakey++", "body"); + assert!(!warnings.is_empty()); + } } diff --git a/src/tui.rs b/src/tui.rs index 275e883..bc71a83 100644 --- a/src/tui.rs +++ b/src/tui.rs @@ -2,7 +2,7 @@ use crate::app::{App, View}; use crate::events::handle_events; use crate::feed::Feed; use crate::ui; -use anyhow::Result; +use anyhow::{Context, Result}; use crossterm::{ event::{self, DisableMouseCapture, EnableMouseCapture}, execute, @@ -12,9 +12,101 @@ use ratatui::{ backend::{Backend, CrosstermBackend}, Terminal, }; +use std::io::Write; +use std::process::{Command, Stdio}; use std::sync::mpsc; use std::{io, time::Duration}; +/// RAII guard that re-enters the alt-screen + raw mode + mouse capture on drop, +/// so a panic during a pipe-to child invocation cannot leave the terminal in +/// a broken state. +struct TerminalRestoreGuard; + +impl TerminalRestoreGuard { + fn enter() -> io::Result { + // Leave the TUI before handing the tty to the child. + let mut stdout = io::stdout(); + execute!(stdout, LeaveAlternateScreen, DisableMouseCapture)?; + disable_raw_mode()?; + Ok(Self) + } +} + +impl Drop for TerminalRestoreGuard { + fn drop(&mut self) { + // Best-effort restore — ignore errors during unwind. + let _ = enable_raw_mode(); + let _ = execute!(io::stdout(), EnterAlternateScreen, EnableMouseCapture); + } +} + +/// Suspend the TUI, run argv with the given stdin payload (or inherited stdin +/// if `stdin_payload` is None), then re-enter the TUI and force a redraw. +/// Drains any pending terminal events so terminal-probe responses from the +/// child don't leak into our key handler. +pub fn suspend_for_command( + terminal: &mut Terminal, + argv: &[String], + stdin_payload: Option<&[u8]>, +) -> Result { + if argv.is_empty() { + anyhow::bail!("empty command"); + } + let status = { + let _guard = TerminalRestoreGuard::enter()?; + let mut cmd = Command::new(&argv[0]); + cmd.args(&argv[1..]); + if stdin_payload.is_some() { + cmd.stdin(Stdio::piped()); + } + // stdout/stderr inherited so user sees output of the child. + let mut child = cmd + .spawn() + .with_context(|| format!("failed to spawn '{}'", argv[0]))?; + if let (Some(payload), Some(mut stdin)) = (stdin_payload, child.stdin.take()) { + stdin.write_all(payload).ok(); + // Dropping `stdin` here closes the pipe so the child sees EOF. + } + child.wait().context("failed to wait for child")? + }; + // Drain any pending input that might have arrived during the child's run + // (e.g. terminal-probe responses from less/vim). + while event::poll(Duration::from_millis(0))? { + let _ = event::read(); + } + terminal.clear()?; + Ok(status) +} + +/// Spawn a child detached: stdio nulled, no wait. The Child is dropped — on +/// Linux the OS reaps it after parent exit. Used by exec_on_new where we +/// fan out potentially many children and must not block the main loop. +pub fn spawn_detached(argv: &[String], stdin_payload: Option>) -> Result<()> { + if argv.is_empty() { + anyhow::bail!("empty command"); + } + let mut cmd = Command::new(&argv[0]); + cmd.args(&argv[1..]) + .stdout(Stdio::null()) + .stderr(Stdio::null()); + if stdin_payload.is_some() { + cmd.stdin(Stdio::piped()); + } else { + cmd.stdin(Stdio::null()); + } + let mut child = cmd + .spawn() + .with_context(|| format!("failed to spawn '{}'", argv[0]))?; + if let (Some(payload), Some(mut stdin)) = (stdin_payload, child.stdin.take()) { + // Best-effort write; closing stdin happens when the handle drops. + std::thread::spawn(move || { + let _ = stdin.write_all(&payload); + }); + } + // Intentionally do not wait — child becomes orphaned, OS reaps on parent exit. + Ok(()) +} + pub fn run(mut app: App) -> Result<()> { // Set up terminal enable_raw_mode()?; @@ -43,6 +135,99 @@ pub fn run(mut app: App) -> Result<()> { Ok(()) } +/// Drain all queued macro steps in FIFO order. Action steps mutate app +/// state inline, PipeTo suspends the terminal for a blocking child, Exec +/// spawns detached. On the first error the rest of the queue is dropped +/// to avoid running a follow-up step in an undefined state. +fn drain_macro_steps(terminal: &mut Terminal, app: &mut App) { + use crate::keybindings::MacroStep; + while !app.pending_macro_steps.is_empty() { + let step = app.pending_macro_steps.remove(0); + match step { + MacroStep::Action(a) => crate::events::dispatch_action(app, a), + MacroStep::PipeTo { + argv_template, + stdin, + } => { + let (argv, payload) = + match crate::events::build_pipe_invocation(app, &argv_template, stdin) { + Some(v) => v, + None => { + app.error = Some("pipe-to: no article in focus".to_string()); + app.pending_macro_steps.clear(); + break; + } + }; + if let Err(e) = suspend_for_command(terminal, &argv, Some(&payload)) { + app.error = Some(format!("pipe-to: {}", e)); + app.pending_macro_steps.clear(); + break; + } + } + MacroStep::Exec { argv_template } => { + let argv = match crate::events::build_exec_invocation(app, &argv_template) { + Some(v) => v, + None => { + app.error = Some("exec: no article in focus".to_string()); + app.pending_macro_steps.clear(); + break; + } + }; + if let Err(e) = spawn_detached(&argv, None) { + app.error = Some(format!("exec: {}", e)); + } + } + } + } +} + +/// Diff a freshly-arrived feed against `app.seen_items` and fire the +/// `[hooks].exec_on_new` template (if configured) for each genuinely-new item. +/// On the first successful fetch of a feed (URL not yet in `feeds_seeded`), +/// seed the seen set silently to suppress the firehose. +fn fire_exec_on_new(app: &mut App, feed: &Feed) { + let newly_seen_idx = crate::app::mark_feed_seen(app, feed); + if newly_seen_idx.is_empty() { + return; + } + let template_str = match app.config.hooks.exec_on_new.as_deref() { + Some(t) if !t.trim().is_empty() => t.to_string(), + _ => return, + }; + let argv_template = match shlex::split(&template_str) { + Some(t) if !t.is_empty() => t, + _ => { + app.error = Some(format!( + "exec_on_new: could not tokenize template: {}", + template_str + )); + return; + } + }; + for idx in newly_seen_idx { + let item = match feed.items.get(idx) { + Some(i) => i, + None => continue, + }; + let ctx = crate::app::ArticleContext { + title: &item.title, + url: item.link.as_deref(), + author: item.author.as_deref(), + formatted_date: item.formatted_date.as_deref(), + feed_title: &feed.title, + feed_url: &feed.url, + plain_text: item.plain_text.as_deref(), + }; + let argv = crate::app::expand_argv_template(&argv_template, &ctx); + if let Err(e) = spawn_detached(&argv, None) { + // Show the first failure; subsequent ones are suppressed. + if app.error.is_none() { + app.error = Some(format!("exec_on_new: {}", e)); + } + } + } +} + /// Spawn background threads to fetch all bookmarked feeds, sending results through the channel. /// Returns the sender's pending count and the receiver. fn spawn_feed_refresh(app: &mut App) -> (usize, mpsc::Receiver<(usize, Result)>) { @@ -107,6 +292,7 @@ fn run_app(terminal: &mut Terminal, app: &mut App) -> Result<()> if pending_count > 0 { while let Ok((idx, result)) = feed_rx.try_recv() { if let Ok(feed) = result { + fire_exec_on_new(app, &feed); // Insert at the correct position to maintain bookmark order, // or append if earlier feeds haven't arrived yet let insert_pos = app @@ -165,6 +351,7 @@ fn run_app(terminal: &mut Terminal, app: &mut App) -> Result<()> if handle_events(app)? { return Ok(()); } + drain_macro_steps(terminal, app); } else if last_tick.elapsed() >= tick_rate { // Update animation frame on tick if app.is_loading { diff --git a/src/ui/modals.rs b/src/ui/modals.rs index 239bbc4..53e533a 100644 --- a/src/ui/modals.rs +++ b/src/ui/modals.rs @@ -1,5 +1,5 @@ use crate::app::{App, InputMode, LinkType, TimeFilter, View}; -use crate::keybindings::{key_display, KeyAction}; +use crate::keybindings::{binding_display, key_display, KeyAction, MacroStep}; use crate::ui::utils::{centered_rect_with_min, truncate_str}; use crate::ui::ColorScheme; use ratatui::{ @@ -803,6 +803,39 @@ pub(super) fn render_help_overlay(f: &mut Frame, app: &App, color } } + if !app.macros.is_empty() { + lines.push(Line::from("")); + lines.push(separator.clone()); + lines.push(Line::from("")); + let prefix_label = binding_display(&app.macro_options.prefix); + lines.push(Line::from(Span::styled( + format!(" Macros (press {} then key)", prefix_label), + section_style, + ))); + lines.push(Line::from("")); + for mac in &app.macros { + let trigger_str = format!("{}{}", prefix_label, binding_display(&mac.trigger)); + let summary = match &mac.description { + Some(d) => d.clone(), + None => mac + .steps + .iter() + .map(|s| match s { + MacroStep::Action(a) => format!("{:?}", a), + MacroStep::PipeTo { argv_template, .. } => { + format!("pipe-to {}", argv_template.join(" ")) + } + MacroStep::Exec { argv_template } => { + format!("exec {}", argv_template.join(" ")) + } + }) + .collect::>() + .join(" ; "), + }; + add_key(&trigger_str, &summary, &mut lines); + } + } + lines.push(Line::from("")); lines.push(separator); lines.push(Line::from("")); From a89604e091e4bce02c66efa2d8f7fb24ef7adf11 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Mon, 11 May 2026 07:16:00 +0200 Subject: [PATCH 2/6] fix: address review feedback on external-command hooks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six bugs and a pile of polish from the principal review on the hooks branch. Nothing here changes the user-facing shape of macros or exec_on_new — it's all the things that go wrong when you stare at the code for an hour. The biggest one: `exec_on_new` could fire twice for the same item after a crash. The reason was that `mark_feed_seen` updates the in-memory seen-set as each feed arrives in a refresh batch, but `save_data` only ran at the end of the batch. Kill feedr mid-batch and the persisted seen-set is stale — next launch, the same item gets re-fired. For a `notify-send` hook that's annoying. For `wallabag-cli add` it's actively wrong. Persist before the spawn loop. At-most-once beats at-least-once when you're firing arbitrary external commands. The next one: the macro prefix had no escape hatch. Press `,` and walk away, and the next keystroke is silently consumed as a macro follow-up — including a `j` you meant as move-down. ESC now cancels cleanly, the prefix wait times out on tick, and unmatched follow-ups still error rather than falling through, because I'd rather surface a typo than silently jump-top. While at it: pre-tokenize the exec_on_new template once at startup instead of re-shlex-ing it for every feed arrival; surface friendly dashed action names (`open-in-browser` not `OpenInBrowser`) in errors and the help overlay; track first-failure with a local bool so the comment matches the code; switch `pending_macro_steps` to a VecDeque so the drain isn't O(n) per step. Tidy the byte-level \` -- \` separator check while I was in there. Inline help in `config.rs` now spells out the supported actions inside macros, warns that \`~\`/\`\$HOME\` are *not* expanded (no shell, no expansion, please don't be surprised), points out that wrapping in \`sh -c\` re-introduces shell injection through item titles, and documents the at-most-once semantics for exec_on_new so users choose idempotent commands. Tests for the new behavior: ESC cancellation, prefix-then-unbound, prefix-then-bound, \`dispatch_action\` rejecting unsupported actions with the friendly name, the \`KeyAction::as_str\` <-> \`FromStr\` round-trip so future variants can't silently drift, and a SavedData round-trip for the new persistence fields plus back-compat with the old shape. 110 tests pass, clippy clean with -D warnings. --- src/app.rs | 97 ++++++++++++++++++++++++++++++++++++++++-- src/config.rs | 40 ++++++++++++++---- src/events.rs | 102 +++++++++++++++++++++++++++++++++++++++++++-- src/keybindings.rs | 101 ++++++++++++++++++++++++++++++++++++++++---- src/tui.rs | 53 +++++++++++------------ src/ui/modals.rs | 2 +- 6 files changed, 345 insertions(+), 50 deletions(-) diff --git a/src/app.rs b/src/app.rs index 7dc4713..3f5a4bc 100644 --- a/src/app.rs +++ b/src/app.rs @@ -4,7 +4,7 @@ use crate::ui::ColorScheme; use anyhow::Result; use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; -use std::collections::{HashMap, HashSet}; +use std::collections::{HashMap, HashSet, VecDeque}; use std::fs; use std::path::{Path, PathBuf}; use std::time::Instant; @@ -124,6 +124,10 @@ pub struct App { pub macro_options: crate::keybindings::MacroOptions, /// True after the macro-prefix key has been pressed and we are awaiting the next key. pub awaiting_macro_key: bool, + /// When `awaiting_macro_key` was last set, so the TUI tick loop can time it + /// out after `macro_prefix_timeout` rather than stranding the prefix state + /// across an idle period. + pub awaiting_macro_key_since: Option, /// Item IDs that exec_on_new has already fired for (persisted). pub seen_items: HashSet, /// Feed URLs whose first successful fetch has been observed (persisted). @@ -134,7 +138,11 @@ pub struct App { /// state) and PipeTo/Exec (which spawn children). Draining happens at /// the loop level because PipeTo needs the terminal handle to suspend /// the TUI, which `handle_events` does not have access to. - pub pending_macro_steps: Vec, + pub pending_macro_steps: VecDeque, + /// Pre-tokenized `[hooks].exec_on_new` template. `None` if the hook is + /// disabled or the template failed to tokenize at startup (in which case + /// a startup warning is surfaced). + pub exec_on_new_template: Option>, } /// Snapshot of the focused article passed to template-expansion / pipe-payload helpers. @@ -265,6 +273,24 @@ impl App { let (macros, mac_warnings) = crate::keybindings::build_macros(&config.macros, ¯o_options); + // Pre-tokenize the exec_on_new template once at startup so we don't + // re-shlex on every feed arrival, and surface tokenization errors as a + // startup warning instead of an unexpected runtime failure later. + let mut hook_warnings: Vec = Vec::new(); + let exec_on_new_template = config + .hooks + .exec_on_new + .as_deref() + .map(str::trim) + .filter(|s| !s.is_empty()) + .and_then(|t| match shlex::split(t) { + Some(toks) if !toks.is_empty() => Some(toks), + _ => { + hook_warnings.push(format!("exec_on_new: could not tokenize: {}", t)); + None + } + }); + let mut app = Self { config, feeds: Vec::new(), @@ -320,9 +346,11 @@ impl App { macros, macro_options, awaiting_macro_key: false, + awaiting_macro_key_since: None, seen_items: saved_data.seen_items, feeds_seeded: saved_data.feeds_seeded, - pending_macro_steps: Vec::new(), + pending_macro_steps: VecDeque::new(), + exec_on_new_template, }; app.update_dashboard(); @@ -331,6 +359,7 @@ impl App { let mut all_warnings = kb_warnings; all_warnings.extend(mo_warnings); all_warnings.extend(mac_warnings); + all_warnings.extend(hook_warnings); if !all_warnings.is_empty() { app.error = Some(format!("Config: {}", all_warnings.join("; "))); } @@ -1651,6 +1680,21 @@ impl App { } } +/// Build an `ArticleContext` directly from a `Feed` + `FeedItem` pair — +/// the path used by `exec_on_new`, where the new item may not yet live +/// inside `App::feeds`. +pub fn article_context_from<'a>(feed: &'a Feed, item: &'a FeedItem) -> ArticleContext<'a> { + ArticleContext { + title: &item.title, + url: item.link.as_deref(), + author: item.author.as_deref(), + formatted_date: item.formatted_date.as_deref(), + feed_title: &feed.title, + feed_url: &feed.url, + plain_text: item.plain_text.as_deref(), + } +} + /// Build the same identifier scheme used by `read_items`/`starred_items` from /// a feed + item that may not yet be in `App::feeds`. pub fn make_item_id(feed: &Feed, item: &FeedItem) -> String { @@ -2409,4 +2453,51 @@ mod tests { item2.title = "T2".to_string(); assert_eq!(make_item_id(&feed, &item2), "https://ex.com/feed.xml_T2"); } + + #[test] + fn test_saved_data_seen_items_feeds_seeded_roundtrip() { + // Verify that the new persistence fields survive a JSON round-trip + // with the same shape the App expects. + let mut seen_items = HashSet::new(); + seen_items.insert("https://ex.com/a".to_string()); + seen_items.insert("https://ex.com/feed.xml_T2".to_string()); + let mut feeds_seeded = HashSet::new(); + feeds_seeded.insert("https://ex.com/feed.xml".to_string()); + + let original = SavedData { + bookmarks: vec!["https://ex.com/feed.xml".to_string()], + categories: vec![], + read_items: HashSet::new(), + starred_items: HashSet::new(), + last_session_time: None, + seen_items: seen_items.clone(), + feeds_seeded: feeds_seeded.clone(), + }; + + let json = serde_json::to_string(&original).unwrap(); + let parsed: SavedData = serde_json::from_str(&json).unwrap(); + + assert_eq!(parsed.seen_items, seen_items); + assert_eq!(parsed.feeds_seeded, feeds_seeded); + } + + #[test] + fn test_saved_data_back_compat_without_new_fields() { + // An older feedr_data.json with no seen_items / feeds_seeded must + // still deserialize (defaults to empty sets), so users who upgrade + // mid-flight don't lose their bookmarks. + let json = r#"{ + "bookmarks": ["https://ex.com/feed.xml"], + "categories": [], + "read_items": [], + "starred_items": [] + }"#; + let parsed: SavedData = serde_json::from_str(json).unwrap(); + assert_eq!( + parsed.bookmarks, + vec!["https://ex.com/feed.xml".to_string()] + ); + assert!(parsed.seen_items.is_empty()); + assert!(parsed.feeds_seeded.is_empty()); + } } diff --git a/src/config.rs b/src/config.rs index 1483e28..4dd1bf5 100644 --- a/src/config.rs +++ b/src/config.rs @@ -409,20 +409,39 @@ impl Config { # ── External-command hooks ──────────────────────────────\n\ #\n\ # Macros bind a key (invoked as , default prefix is ',') to\n\ - # an ordered chain of actions. Steps are separated by ';'. Recognized\n\ - # action names mirror the existing keybinding actions (use dashes), plus\n\ - # the synthetic ops `pipe-to` and `exec`. Variables expanded per token:\n\ - # %t title %u url %a author %d date %f feed-title %F feed-url\n\ + # an ordered chain of steps. Steps are separated by ';'. A trailing\n\ + # ` -- \"description\"` overrides the help-overlay label.\n\ #\n\ - # IMPORTANT: command templates are NOT run through a shell. They are\n\ - # tokenized once via shell-style splitting and %X is substituted into\n\ - # individual argv tokens. If you need pipes or redirection, write a\n\ - # shell script and invoke that.\n\ + # Step kinds:\n\ + # run a built-in action (see list below)\n\ + # pipe-to \"cmd %u\" [stdin=…] run `cmd %u` with article content on stdin\n\ + # exec \"cmd %u\" run `cmd %u` with no stdin\n\ + #\n\ + # Supported actions inside macros:\n\ + # open-in-browser, toggle-star, toggle-read, mark-all-read,\n\ + # refresh, toggle-theme, extract-links, help\n\ + # Other keybinding actions are intentionally not callable from macros.\n\ + #\n\ + # Variables expanded per argv token:\n\ + # %t title %u url %a author %d date %f feed-title %F feed-url %% literal %\n\ + #\n\ + # IMPORTANT — commands are NOT run through a shell:\n\ + # * Templates are tokenized once and %X is substituted into argv tokens.\n\ + # Article content cannot break out of an argument.\n\ + # * For pipes, redirection, or globbing, write a small shell script\n\ + # and invoke that.\n\ + # * `~` and `$HOME` / `$VAR` are NOT expanded — use absolute paths.\n\ + # * Wrapping your command in `sh -c \"... %t ...\"` REINTRODUCES shell\n\ + # injection through item titles. Prefer a script file.\n\ + #\n\ + # Quoting: the outer parser is shell-style, so to put a quoted token in\n\ + # the inner argv, escape twice:\n\ + # pipe-to \"echo \\\"hello world\\\"\"\n\ #\n\ # [macros]\n\ # y = 'open-in-browser ; pipe-to \"yt-dlp %u\"'\n\ # w = 'pipe-to \"wallabag-cli add %u\" -- \"Save to Wallabag\"'\n\ - # n = 'pipe-to \"tee out.txt\" stdin=metadata'\n\ + # n = 'pipe-to \"tee /tmp/out.txt\" stdin=metadata'\n\ #\n\ # [macro_options]\n\ # prefix = \",\" # the macro-prefix key\n\ @@ -431,6 +450,9 @@ impl Config { # The exec_on_new hook fires once per newly-seen item on each refresh.\n\ # The first successful fetch of a feed seeds the seen set silently to\n\ # avoid a firehose on initial load. Children are spawned detached.\n\ + # Semantics on crash are AT MOST ONCE — feedr persists the seen-set\n\ + # before spawning, so a kill mid-fire loses a notification rather than\n\ + # firing again on the next launch. Prefer idempotent commands.\n\ #\n\ # [hooks]\n\ # exec_on_new = 'notify-send \"New: %t\" \"%f\"'\n", diff --git a/src/events.rs b/src/events.rs index 1bbcf29..aa4b26a 100644 --- a/src/events.rs +++ b/src/events.rs @@ -104,7 +104,7 @@ fn handle_toggle_read_current(app: &mut App) { /// Queue every step of a macro for the TUI drain in order. fn run_macro(app: &mut App, mac: &MacroBinding) { for step in &mac.steps { - app.pending_macro_steps.push(step.clone()); + app.pending_macro_steps.push_back(step.clone()); } } @@ -161,8 +161,8 @@ pub(crate) fn dispatch_action(app: &mut App, action: KeyAction) { KeyAction::ExtractLinks => app.extract_links_from_current_item(), other => { app.error = Some(format!( - "macro: action '{:?}' not supported in macros", - other + "macro: action '{}' not supported in macros", + other.as_str() )); } } @@ -250,6 +250,14 @@ pub(crate) fn handle_key_event(app: &mut App, key: crossterm::event::KeyEvent) - if app.input_mode == InputMode::Normal { if app.awaiting_macro_key { app.awaiting_macro_key = false; + app.awaiting_macro_key_since = None; + // Clear the "Macro prefix..." hint so it doesn't outlive the wait. + app.success_message = None; + app.success_message_time = None; + // ESC explicitly cancels; the key is consumed and nothing else fires. + if key.code == KeyCode::Esc { + return Ok(false); + } match app.macro_for_key(&key) { Some(mac) => { let mac = mac.clone(); @@ -262,9 +270,11 @@ pub(crate) fn handle_key_event(app: &mut App, key: crossterm::event::KeyEvent) - return Ok(false); } if app.macro_options.prefix.matches(&key) && !app.macros.is_empty() { + let now = std::time::Instant::now(); app.awaiting_macro_key = true; + app.awaiting_macro_key_since = Some(now); app.success_message = Some("Macro prefix... press macro key".to_string()); - app.success_message_time = Some(std::time::Instant::now()); + app.success_message_time = Some(now); return Ok(false); } } @@ -1920,4 +1930,88 @@ mod tests { assert!(!result); assert_eq!(app.view, View::FeedList); } + + // ── Macro engine tests ──────────────────────────────────────── + + #[test] + fn test_dispatch_action_rejects_unsupported_action() { + let mut app = make_test_app(); + // MoveUp is intentionally not callable from macros — verify the error + // surfaces the friendly dashed name, not a debug-formatted variant. + dispatch_action(&mut app, KeyAction::MoveUp); + let err = app.error.as_deref().unwrap_or(""); + assert!(err.contains("move-up"), "got error: {}", err); + assert!(err.contains("not supported"), "got error: {}", err); + } + + #[test] + fn test_macro_prefix_esc_cancels_cleanly() { + let mut app = make_test_app(); + // Bind a macro so the prefix is active. + app.macros = vec![crate::keybindings::MacroBinding { + trigger: crate::keybindings::KeyBinding::new(KeyCode::Char('y')), + steps: vec![crate::keybindings::MacroStep::Action(KeyAction::Refresh)], + description: None, + }]; + + // Press the prefix. + let prefix = make_key(KeyCode::Char(','), KeyModifiers::NONE); + handle_key_event(&mut app, prefix).unwrap(); + assert!(app.awaiting_macro_key); + + // ESC must cancel without firing anything and without surfacing an error. + let esc = make_key(KeyCode::Esc, KeyModifiers::NONE); + handle_key_event(&mut app, esc).unwrap(); + assert!(!app.awaiting_macro_key); + assert!(app.awaiting_macro_key_since.is_none()); + assert!( + app.error.is_none(), + "ESC must not produce an error, got: {:?}", + app.error + ); + assert!(app.pending_macro_steps.is_empty()); + } + + #[test] + fn test_macro_prefix_unknown_followup_errors_but_clears_state() { + let mut app = make_test_app(); + app.macros = vec![crate::keybindings::MacroBinding { + trigger: crate::keybindings::KeyBinding::new(KeyCode::Char('y')), + steps: vec![crate::keybindings::MacroStep::Action(KeyAction::Refresh)], + description: None, + }]; + + let prefix = make_key(KeyCode::Char(','), KeyModifiers::NONE); + handle_key_event(&mut app, prefix).unwrap(); + // Press an unbound follow-up key. We surface an error rather than + // silently falling through to normal dispatch, but the prefix state + // must be cleared so subsequent keystrokes work normally. + let z = make_key(KeyCode::Char('z'), KeyModifiers::NONE); + handle_key_event(&mut app, z).unwrap(); + assert!(!app.awaiting_macro_key); + assert!(app.awaiting_macro_key_since.is_none()); + assert!(app.error.is_some()); + } + + #[test] + fn test_macro_prefix_then_bound_key_queues_steps() { + let mut app = make_test_app(); + app.macros = vec![crate::keybindings::MacroBinding { + trigger: crate::keybindings::KeyBinding::new(KeyCode::Char('y')), + steps: vec![ + crate::keybindings::MacroStep::Action(KeyAction::ToggleStar), + crate::keybindings::MacroStep::Exec { + argv_template: vec!["true".to_string()], + }, + ], + description: None, + }]; + + let prefix = make_key(KeyCode::Char(','), KeyModifiers::NONE); + handle_key_event(&mut app, prefix).unwrap(); + let y = make_key(KeyCode::Char('y'), KeyModifiers::NONE); + handle_key_event(&mut app, y).unwrap(); + assert!(!app.awaiting_macro_key); + assert_eq!(app.pending_macro_steps.len(), 2); + } } diff --git a/src/keybindings.rs b/src/keybindings.rs index c4128c6..30aeafa 100644 --- a/src/keybindings.rs +++ b/src/keybindings.rs @@ -108,6 +108,47 @@ pub enum KeyAction { PrevTab, } +impl KeyAction { + /// Return the dashed lowercase name a user would write in their config + /// (the inverse of `FromStr`, modulo `-` vs `_`). + pub fn as_str(&self) -> &'static str { + match self { + Self::Quit => "quit", + Self::ForceQuit => "force-quit", + Self::Back => "back", + Self::Home => "home", + Self::ToggleTheme => "toggle-theme", + Self::Refresh => "refresh", + Self::Help => "help", + Self::OpenSearch => "open-search", + Self::MoveUp => "move-up", + Self::MoveDown => "move-down", + Self::PageUp => "page-up", + Self::PageDown => "page-down", + Self::JumpTop => "jump-top", + Self::JumpBottom => "jump-bottom", + Self::Select => "select", + Self::AddFeed => "add-feed", + Self::DeleteFeed => "delete-feed", + Self::ToggleRead => "toggle-read", + Self::ToggleStar => "toggle-star", + Self::MarkAllRead => "mark-all-read", + Self::OpenInBrowser => "open-in-browser", + Self::TogglePreview => "toggle-preview", + Self::OpenFilter => "open-filter", + Self::CycleCategory => "cycle-category", + Self::OpenCategoryManagement => "open-category-management", + Self::AssignCategory => "assign-category", + Self::ExtractLinks => "extract-links", + Self::ScrollPreviewUp => "scroll-preview-up", + Self::ScrollPreviewDown => "scroll-preview-down", + Self::ToggleExpand => "toggle-expand", + Self::NextTab => "next-tab", + Self::PrevTab => "prev-tab", + } + } +} + impl FromStr for KeyAction { type Err = (); @@ -524,13 +565,10 @@ fn split_description(s: &str) -> (&str, Option) { i += 1; continue; } - if !in_quote - && b == b' ' - && i + 3 < bytes.len() - && bytes[i + 1] == b'-' - && bytes[i + 2] == b'-' - && bytes[i + 3] == b' ' - { + // Look for the literal byte sequence ` -- ` (space-dash-dash-space) + // outside quoted spans. `b == b' '` is the leading space; the rest of + // the separator is matched with a single slice compare. + if !in_quote && b == b' ' && bytes.get(i + 1..=i + 3) == Some(b"-- ") { // i and i+4 are ASCII boundaries; safe to slice. let body = &s[..i]; let desc_part = s[i + 4..].trim(); @@ -1043,4 +1081,53 @@ mod tests { let (_, warnings) = resolve_macro_options("notakey++", "body"); assert!(!warnings.is_empty()); } + + #[test] + fn test_keyaction_as_str_roundtrip_with_fromstr() { + // Every KeyAction must round-trip through as_str() -> "-"→"_" -> from_str(). + // If you add a new KeyAction variant, add it to both maps. + let all = [ + KeyAction::Quit, + KeyAction::ForceQuit, + KeyAction::Back, + KeyAction::Home, + KeyAction::ToggleTheme, + KeyAction::Refresh, + KeyAction::Help, + KeyAction::OpenSearch, + KeyAction::MoveUp, + KeyAction::MoveDown, + KeyAction::PageUp, + KeyAction::PageDown, + KeyAction::JumpTop, + KeyAction::JumpBottom, + KeyAction::Select, + KeyAction::AddFeed, + KeyAction::DeleteFeed, + KeyAction::ToggleRead, + KeyAction::ToggleStar, + KeyAction::MarkAllRead, + KeyAction::OpenInBrowser, + KeyAction::TogglePreview, + KeyAction::OpenFilter, + KeyAction::CycleCategory, + KeyAction::OpenCategoryManagement, + KeyAction::AssignCategory, + KeyAction::ExtractLinks, + KeyAction::ScrollPreviewUp, + KeyAction::ScrollPreviewDown, + KeyAction::ToggleExpand, + KeyAction::NextTab, + KeyAction::PrevTab, + ]; + for a in all { + let s = a.as_str(); + // Display form uses dashes for readability. + assert!(!s.contains('_'), "as_str() should use dashes, got: {}", s); + let parsed = KeyAction::from_str(&s.replace('-', "_")).unwrap_or_else(|_| { + panic!("could not round-trip KeyAction::{:?} via as_str()={}", a, s) + }); + assert_eq!(parsed, a); + } + } } diff --git a/src/tui.rs b/src/tui.rs index bc71a83..469cd31 100644 --- a/src/tui.rs +++ b/src/tui.rs @@ -141,8 +141,7 @@ pub fn run(mut app: App) -> Result<()> { /// to avoid running a follow-up step in an undefined state. fn drain_macro_steps(terminal: &mut Terminal, app: &mut App) { use crate::keybindings::MacroStep; - while !app.pending_macro_steps.is_empty() { - let step = app.pending_macro_steps.remove(0); + while let Some(step) = app.pending_macro_steps.pop_front() { match step { MacroStep::Action(a) => crate::events::dispatch_action(app, a), MacroStep::PipeTo { @@ -185,44 +184,37 @@ fn drain_macro_steps(terminal: &mut Terminal, app: &mut App) { /// `[hooks].exec_on_new` template (if configured) for each genuinely-new item. /// On the first successful fetch of a feed (URL not yet in `feeds_seeded`), /// seed the seen set silently to suppress the firehose. +/// +/// Persistence order is "mark-seen, persist, spawn" so semantics on crash are +/// AT MOST ONCE: a kill between persist and spawn loses a notification, but a +/// later restart never re-fires the same item. This matters for side-effecting +/// hooks like `wallabag-cli add`. fn fire_exec_on_new(app: &mut App, feed: &Feed) { let newly_seen_idx = crate::app::mark_feed_seen(app, feed); if newly_seen_idx.is_empty() { return; } - let template_str = match app.config.hooks.exec_on_new.as_deref() { - Some(t) if !t.trim().is_empty() => t.to_string(), - _ => return, - }; - let argv_template = match shlex::split(&template_str) { - Some(t) if !t.is_empty() => t, - _ => { - app.error = Some(format!( - "exec_on_new: could not tokenize template: {}", - template_str - )); - return; - } + // Persist the updated seen set before spawning. If we crash before + // spawning, the user misses a notification — preferable to a duplicate + // for side-effecting hooks. + let _ = app.save_data(); + + let argv_template = match app.exec_on_new_template.as_ref() { + Some(t) => t.clone(), + None => return, }; + let mut first_failure_recorded = false; for idx in newly_seen_idx { let item = match feed.items.get(idx) { Some(i) => i, None => continue, }; - let ctx = crate::app::ArticleContext { - title: &item.title, - url: item.link.as_deref(), - author: item.author.as_deref(), - formatted_date: item.formatted_date.as_deref(), - feed_title: &feed.title, - feed_url: &feed.url, - plain_text: item.plain_text.as_deref(), - }; + let ctx = crate::app::article_context_from(feed, item); let argv = crate::app::expand_argv_template(&argv_template, &ctx); if let Err(e) = spawn_detached(&argv, None) { - // Show the first failure; subsequent ones are suppressed. - if app.error.is_none() { + if !first_failure_recorded { app.error = Some(format!("exec_on_new: {}", e)); + first_failure_recorded = true; } } } @@ -372,6 +364,15 @@ fn run_app(terminal: &mut Terminal, app: &mut App) -> Result<()> } } + // Time out a stranded macro-prefix wait so the next unrelated + // keystroke isn't silently consumed as a macro follow-up. + if let Some(since) = app.awaiting_macro_key_since { + if since.elapsed() >= success_timeout { + app.awaiting_macro_key = false; + app.awaiting_macro_key_since = None; + } + } + // Check if auto-refresh should trigger if app.should_auto_refresh() { app.refresh_requested = true; diff --git a/src/ui/modals.rs b/src/ui/modals.rs index 53e533a..2261dcb 100644 --- a/src/ui/modals.rs +++ b/src/ui/modals.rs @@ -821,7 +821,7 @@ pub(super) fn render_help_overlay(f: &mut Frame, app: &App, color .steps .iter() .map(|s| match s { - MacroStep::Action(a) => format!("{:?}", a), + MacroStep::Action(a) => a.as_str().to_string(), MacroStep::PipeTo { argv_template, .. } => { format!("pipe-to {}", argv_template.join(" ")) } From 1c65e4e4a11255a98989332758a69774710149de Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Mon, 11 May 2026 09:06:38 +0200 Subject: [PATCH 3/6] fix: address second-round review on external-command hooks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A code review on top of the first review-feedback commit turned up three things that are *not* OK to ship, plus a few smaller ones. The big one: dispatch_action read app.selected_feed for ToggleStar / ToggleRead, but selected_feed is only ever set when you drill into FeedItems or FeedItemDetail. So a macro like `,s = toggle-star` silently no-op'd on Dashboard and Starred views. The per-key handlers in those views *do* resolve the focused item correctly via active_dashboard_items() / get_starred_dashboard_items(), but the macro path was happily ignoring all of that. Two entry points, two answers for "which item is focused." Not great. Add App::current_article_indices() as the single source of truth, have current_article_context() delegate to it, and route the toggle helpers through it. Now keypress and macro agree on what "current" means. The other big one: spawn_detached spawned a child and dropped the Child without ever waiting on it. On Unix this leaves the kernel holding a zombie until the parent exits. Feedr is a long-running TUI; a user with exec_on_new = 'notify-send …' and a handful of active feeds would accumulate processes indefinitely. Stick the Child in a reaper thread that just waits. One thread per child is fine for our scale and it's cross-platform. While at it: fire_exec_on_new was mutating seen_items and calling save_data() on *every* feed arrival even when no hook was configured, which means users who never opted in were paying for an unbounded growing set on disk for absolutely no reason. Short- circuit at the top when the template is None. Also: suspend_for_command was writing the stdin payload synchronously on the main thread before waiting on the child, which can deadlock if the payload is bigger than the pipe buffer (64 KiB on Linux) and the child hasn't started reading yet. Spawn a writer thread, let it run in parallel with child.wait(). Drop the dead stdin_payload parameter from spawn_detached while I'm in there — both call sites pass None. Add regression tests for the two dispatch_action cases. Fix a stale doc comment referencing a macro_prefix_timeout config that doesn't exist. --- src/app.rs | 35 +++++++++------- src/events.rs | 113 ++++++++++++++++++++++++++++++++++++-------------- src/tui.rs | 64 ++++++++++++++++------------ 3 files changed, 141 insertions(+), 71 deletions(-) diff --git a/src/app.rs b/src/app.rs index 3f5a4bc..c222579 100644 --- a/src/app.rs +++ b/src/app.rs @@ -124,9 +124,9 @@ pub struct App { pub macro_options: crate::keybindings::MacroOptions, /// True after the macro-prefix key has been pressed and we are awaiting the next key. pub awaiting_macro_key: bool, - /// When `awaiting_macro_key` was last set, so the TUI tick loop can time it - /// out after `macro_prefix_timeout` rather than stranding the prefix state - /// across an idle period. + /// When `awaiting_macro_key` was last set, so the TUI tick loop can time + /// it out (reusing the 1.5 s success-message timeout) rather than + /// stranding the prefix state across an idle period. pub awaiting_macro_key_since: Option, /// Item IDs that exec_on_new has already fired for (persisted). pub seen_items: HashSet, @@ -1631,22 +1631,23 @@ impl App { } } - /// Resolve the focused article based on the current view + selection. - /// Returns None if nothing is selected or the indices don't resolve. - pub fn current_article_context(&self) -> Option> { - let (feed_idx, item_idx) = match self.view { + /// Resolve `(feed_idx, item_idx)` for the focused article in the current + /// view. Dashboard and Starred views derive the indices from their derived + /// item list rather than from `selected_feed` (which is only populated + /// once the user drills into FeedItems / FeedItemDetail). This is the + /// single source of truth for "which item is focused right now". + pub fn current_article_indices(&self) -> Option<(usize, usize)> { + match self.view { View::Dashboard => { let sel = self.selected_item?; let active = self.active_dashboard_items(); if sel >= active.len() { return None; } - active[sel] + Some(active[sel]) } View::FeedItems | View::FeedItemDetail => { - let feed_idx = self.selected_feed?; - let item_idx = self.selected_item?; - (feed_idx, item_idx) + Some((self.selected_feed?, self.selected_item?)) } View::Starred => { let sel = self.selected_item?; @@ -1654,10 +1655,16 @@ impl App { if sel >= starred.len() { return None; } - starred[sel] + Some(starred[sel]) } - _ => return None, - }; + _ => None, + } + } + + /// Resolve the focused article based on the current view + selection. + /// Returns None if nothing is selected or the indices don't resolve. + pub fn current_article_context(&self) -> Option> { + let (feed_idx, item_idx) = self.current_article_indices()?; let feed = self.feeds.get(feed_idx)?; let item = feed.items.get(item_idx)?; Some(ArticleContext { diff --git a/src/events.rs b/src/events.rs index aa4b26a..d6dbc0b 100644 --- a/src/events.rs +++ b/src/events.rs @@ -53,41 +53,39 @@ fn handle_show_help(app: &mut App) { } fn handle_toggle_star_current(app: &mut App) { - if let Some(feed_idx) = app.selected_feed { - if let Some(item_idx) = app.selected_item { - match app.toggle_item_starred(feed_idx, item_idx) { - Ok(is_now_starred) => { - app.success_message = Some(if is_now_starred { - "\u{2605} Starred".to_string() - } else { - "\u{2606} Unstarred".to_string() - }); - app.success_message_time = Some(std::time::Instant::now()); - } - Err(e) => { - app.error = Some(format!("Failed to toggle star: {}", e)); - } - } + let Some((feed_idx, item_idx)) = app.current_article_indices() else { + return; + }; + match app.toggle_item_starred(feed_idx, item_idx) { + Ok(is_now_starred) => { + app.success_message = Some(if is_now_starred { + "\u{2605} Starred".to_string() + } else { + "\u{2606} Unstarred".to_string() + }); + app.success_message_time = Some(std::time::Instant::now()); + } + Err(e) => { + app.error = Some(format!("Failed to toggle star: {}", e)); } } } fn handle_toggle_read_current(app: &mut App) { - if let Some(feed_idx) = app.selected_feed { - if let Some(item_idx) = app.selected_item { - match app.toggle_item_read(feed_idx, item_idx) { - Ok(is_now_read) => { - app.success_message = Some(if is_now_read { - "\u{2713} Marked as read".to_string() - } else { - "\u{25CB} Marked as unread".to_string() - }); - app.success_message_time = Some(std::time::Instant::now()); - } - Err(e) => { - app.error = Some(format!("Failed to toggle read status: {}", e)); - } - } + let Some((feed_idx, item_idx)) = app.current_article_indices() else { + return; + }; + match app.toggle_item_read(feed_idx, item_idx) { + Ok(is_now_read) => { + app.success_message = Some(if is_now_read { + "\u{2713} Marked as read".to_string() + } else { + "\u{25CB} Marked as unread".to_string() + }); + app.success_message_time = Some(std::time::Instant::now()); + } + Err(e) => { + app.error = Some(format!("Failed to toggle read status: {}", e)); } } } @@ -2014,4 +2012,59 @@ mod tests { assert!(!app.awaiting_macro_key); assert_eq!(app.pending_macro_steps.len(), 2); } + + #[test] + fn test_dispatch_toggle_star_works_in_dashboard_view() { + // Regression: dispatch_action used to read app.selected_feed, which is + // None in Dashboard view until the user drills into an item. The macro + // path silently no-op'd. After the fix, the focused dashboard item is + // resolved via current_article_indices() and toggling works. + let mut app = make_test_app(); + app.read_items.clear(); + app.starred_items.clear(); + app.view = View::Dashboard; + app.selected_item = Some(0); + assert!(app.selected_feed.is_none(), "precondition for regression"); + assert!( + !app.active_dashboard_items().is_empty(), + "make_test_app populates the dashboard" + ); + + dispatch_action(&mut app, KeyAction::ToggleStar); + + assert_eq!( + app.starred_items.len(), + 1, + "expected item to be starred via dispatch_action" + ); + assert!(app.error.is_none(), "should not error: {:?}", app.error); + } + + #[test] + fn test_dispatch_toggle_read_works_in_starred_view() { + // Regression: same root cause as the Dashboard case — Starred view + // also leaves selected_feed unset. + let mut app = make_test_app(); + app.read_items.clear(); + app.starred_items.clear(); + // Star one item so the Starred view has something to focus on. + app.toggle_item_starred(0, 0).unwrap(); + app.view = View::Starred; + app.selected_item = Some(0); + app.selected_feed = None; + assert_eq!( + app.get_starred_dashboard_items().len(), + 1, + "Starred view has one item to focus" + ); + + dispatch_action(&mut app, KeyAction::ToggleRead); + + assert_eq!( + app.read_items.len(), + 1, + "expected item to be marked read via dispatch_action" + ); + assert!(app.error.is_none(), "should not error: {:?}", app.error); + } } diff --git a/src/tui.rs b/src/tui.rs index 469cd31..d2eb1e2 100644 --- a/src/tui.rs +++ b/src/tui.rs @@ -63,11 +63,24 @@ pub fn suspend_for_command( let mut child = cmd .spawn() .with_context(|| format!("failed to spawn '{}'", argv[0]))?; - if let (Some(payload), Some(mut stdin)) = (stdin_payload, child.stdin.take()) { - stdin.write_all(payload).ok(); - // Dropping `stdin` here closes the pipe so the child sees EOF. + // Write the stdin payload from a dedicated thread so a payload larger + // than the pipe buffer (typically 64 KiB on Linux) cannot deadlock the + // parent against a child that hasn't started reading yet. + let writer = match (stdin_payload, child.stdin.take()) { + (Some(payload), Some(mut stdin)) => { + let payload = payload.to_vec(); + Some(std::thread::spawn(move || { + let _ = stdin.write_all(&payload); + // stdin drops here, closing the pipe so the child sees EOF. + })) + } + _ => None, + }; + let status = child.wait().context("failed to wait for child")?; + if let Some(w) = writer { + let _ = w.join(); } - child.wait().context("failed to wait for child")? + status }; // Drain any pending input that might have arrived during the child's run // (e.g. terminal-probe responses from less/vim). @@ -78,32 +91,26 @@ pub fn suspend_for_command( Ok(status) } -/// Spawn a child detached: stdio nulled, no wait. The Child is dropped — on -/// Linux the OS reaps it after parent exit. Used by exec_on_new where we -/// fan out potentially many children and must not block the main loop. -pub fn spawn_detached(argv: &[String], stdin_payload: Option>) -> Result<()> { +/// Spawn a child detached: stdio nulled, parent does not block. A reaper +/// thread waits on the child so it doesn't linger as a zombie for the +/// lifetime of the TUI session — `Child` left undropped on Unix would +/// otherwise stay `` until parent exit. Used by exec_on_new where +/// we may fan out many children and must not block the main loop. +pub fn spawn_detached(argv: &[String]) -> Result<()> { if argv.is_empty() { anyhow::bail!("empty command"); } let mut cmd = Command::new(&argv[0]); cmd.args(&argv[1..]) + .stdin(Stdio::null()) .stdout(Stdio::null()) .stderr(Stdio::null()); - if stdin_payload.is_some() { - cmd.stdin(Stdio::piped()); - } else { - cmd.stdin(Stdio::null()); - } let mut child = cmd .spawn() .with_context(|| format!("failed to spawn '{}'", argv[0]))?; - if let (Some(payload), Some(mut stdin)) = (stdin_payload, child.stdin.take()) { - // Best-effort write; closing stdin happens when the handle drops. - std::thread::spawn(move || { - let _ = stdin.write_all(&payload); - }); - } - // Intentionally do not wait — child becomes orphaned, OS reaps on parent exit. + std::thread::spawn(move || { + let _ = child.wait(); + }); Ok(()) } @@ -172,7 +179,7 @@ fn drain_macro_steps(terminal: &mut Terminal, app: &mut App) { break; } }; - if let Err(e) = spawn_detached(&argv, None) { + if let Err(e) = spawn_detached(&argv) { app.error = Some(format!("exec: {}", e)); } } @@ -189,7 +196,15 @@ fn drain_macro_steps(terminal: &mut Terminal, app: &mut App) { /// AT MOST ONCE: a kill between persist and spawn loses a notification, but a /// later restart never re-fires the same item. This matters for side-effecting /// hooks like `wallabag-cli add`. +/// +/// No-op when the hook is not configured. seen_items / feeds_seeded only +/// exist to drive this hook, so users who never opt in pay neither the +/// memory cost nor the per-feed save_data round-trip. fn fire_exec_on_new(app: &mut App, feed: &Feed) { + let argv_template = match app.exec_on_new_template.as_ref() { + Some(t) => t.clone(), + None => return, + }; let newly_seen_idx = crate::app::mark_feed_seen(app, feed); if newly_seen_idx.is_empty() { return; @@ -198,11 +213,6 @@ fn fire_exec_on_new(app: &mut App, feed: &Feed) { // spawning, the user misses a notification — preferable to a duplicate // for side-effecting hooks. let _ = app.save_data(); - - let argv_template = match app.exec_on_new_template.as_ref() { - Some(t) => t.clone(), - None => return, - }; let mut first_failure_recorded = false; for idx in newly_seen_idx { let item = match feed.items.get(idx) { @@ -211,7 +221,7 @@ fn fire_exec_on_new(app: &mut App, feed: &Feed) { }; let ctx = crate::app::article_context_from(feed, item); let argv = crate::app::expand_argv_template(&argv_template, &ctx); - if let Err(e) = spawn_detached(&argv, None) { + if let Err(e) = spawn_detached(&argv) { if !first_failure_recorded { app.error = Some(format!("exec_on_new: {}", e)); first_failure_recorded = true; From 9f08e31d838811183223eb0f74e63b5bc421ae92 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Wed, 13 May 2026 17:28:18 +0200 Subject: [PATCH 4/6] fix(macros): halt chain on first step error consistently MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit drain_macro_steps documented "on first error the rest of the queue is dropped" but only PipeTo spawn failures actually obeyed: Exec spawn failures and Action errors (e.g. open-in-browser with no URL) silently continued through the chain. A chain like `open-in-browser ; toggle-star` should not silently toggle-star when the browser launch failed — the user only sees the error and would not expect side-effecting follow-up steps to have run. Track app.error before each step and clear/break the queue when a step introduces a new error. Same break applied to Exec spawn failures for parity with PipeTo. Three drain-path tests added covering halt-on-action-error, clean-chain runs every step, and that a preexisting error isn't mistaken for a step-introduced failure. --- src/tui.rs | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 102 insertions(+), 2 deletions(-) diff --git a/src/tui.rs b/src/tui.rs index d2eb1e2..7ba20f6 100644 --- a/src/tui.rs +++ b/src/tui.rs @@ -144,11 +144,19 @@ pub fn run(mut app: App) -> Result<()> { /// Drain all queued macro steps in FIFO order. Action steps mutate app /// state inline, PipeTo suspends the terminal for a blocking child, Exec -/// spawns detached. On the first error the rest of the queue is dropped -/// to avoid running a follow-up step in an undefined state. +/// spawns detached. On the first failure of any step the rest of the queue +/// is dropped — a "failure" is any condition that surfaces `app.error` +/// (spawn failures, missing article context, or an Action that sets the +/// error itself, e.g. `open-in-browser` with no URL). PipeTo non-zero exits +/// are not treated as failures: a script that returns 1 to mean "skip" is a +/// reasonable usage, and the user already saw the child's output. fn drain_macro_steps(terminal: &mut Terminal, app: &mut App) { use crate::keybindings::MacroStep; while let Some(step) = app.pending_macro_steps.pop_front() { + // Track whether this step introduced a new error so a chain like + // `open-in-browser ; toggle-star` halts when `open-in-browser` fails, + // matching the contract documented above and the PipeTo / Exec paths. + let pre_error = app.error.is_some(); match step { MacroStep::Action(a) => crate::events::dispatch_action(app, a), MacroStep::PipeTo { @@ -181,9 +189,15 @@ fn drain_macro_steps(terminal: &mut Terminal, app: &mut App) { }; if let Err(e) = spawn_detached(&argv) { app.error = Some(format!("exec: {}", e)); + app.pending_macro_steps.clear(); + break; } } } + if !pre_error && app.error.is_some() { + app.pending_macro_steps.clear(); + break; + } } } @@ -392,3 +406,89 @@ fn run_app(terminal: &mut Terminal, app: &mut App) -> Result<()> } } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::keybindings::{KeyAction, MacroStep}; + use ratatui::backend::TestBackend; + + fn test_terminal() -> Terminal { + Terminal::new(TestBackend::new(80, 24)).unwrap() + } + + #[test] + fn test_drain_halts_when_action_sets_error() { + // A chain where step 1 errors must not run step 2. MoveUp is not + // supported in macros (sets app.error to a "not supported" message); + // OpenInBrowser with no focused article sets a different error. With + // halt-on-error the first message survives, because step 2 never runs. + // Without the halt, step 2 would overwrite app.error and a follow-up + // side-effecting step would run silently. + let mut app = App::new(); + app.error = None; + app.pending_macro_steps + .push_back(MacroStep::Action(KeyAction::MoveUp)); + app.pending_macro_steps + .push_back(MacroStep::Action(KeyAction::OpenInBrowser)); + + let mut terminal = test_terminal(); + drain_macro_steps(&mut terminal, &mut app); + + let err = app.error.as_deref().unwrap_or(""); + assert!( + err.contains("move-up") && err.contains("not supported"), + "first error must survive; got: {}", + err + ); + assert!( + !err.contains("open-in-browser"), + "step 2 must not have run (would have overwritten the error); got: {}", + err + ); + assert!(app.pending_macro_steps.is_empty()); + } + + #[test] + fn test_drain_continues_when_steps_succeed() { + // Sanity: a clean chain runs every step. Two `help` steps are no-ops + // that don't set app.error; both must execute and leave the queue empty. + let mut app = App::new(); + app.error = None; + app.show_help_overlay = false; + app.pending_macro_steps + .push_back(MacroStep::Action(KeyAction::Help)); + app.pending_macro_steps + .push_back(MacroStep::Action(KeyAction::Help)); + + let mut terminal = test_terminal(); + drain_macro_steps(&mut terminal, &mut app); + + assert!(app.pending_macro_steps.is_empty()); + assert!(app.error.is_none()); + assert!(app.show_help_overlay); + } + + #[test] + fn test_drain_preserves_preexisting_error_and_still_runs_step() { + // Edge case: if `app.error` is already set before a step runs, the + // step itself didn't set it — we must not treat it as a step failure. + // (In practice this can't happen because handle_events clears app.error + // on the next keypress, but the drain function shouldn't depend on that + // invariant.) + let mut app = App::new(); + app.error = Some("stale error".to_string()); + app.show_help_overlay = false; + app.pending_macro_steps + .push_back(MacroStep::Action(KeyAction::Help)); + app.pending_macro_steps + .push_back(MacroStep::Action(KeyAction::Help)); + + let mut terminal = test_terminal(); + drain_macro_steps(&mut terminal, &mut app); + + // Both steps ran despite the preexisting error. + assert!(app.pending_macro_steps.is_empty()); + assert!(app.show_help_overlay); + } +} From 5724ec9291b56660beffbdf7bdf34ccd01ea5e15 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Wed, 13 May 2026 17:53:08 +0200 Subject: [PATCH 5/6] fix: address third-round review on external-command hooks Four things turned up on the third pass through the hooks branch. Two of them were latent bugs the earlier reviews missed; two were the kind of write-amplification and resource-leak issues that don't bite anyone in a 100-test suite but get embarrassing in production. The biggest one: a `,r = toggle-read` macro on the Dashboard with "unread only" filtered didn't drop the just-read item from the list. The Dashboard keypress called `apply_filters()` inline; the macro path went through `dispatch_action -> handle_toggle_read_current` and silently skipped it. Two execution paths, two answers for "did the dashboard refresh." Move `apply_filters()` into the helper so every caller -- macro, FeedItems, FeedItemDetail -- agrees. Next: `mark_feed_seen` flipped `feeds_seeded` even when the first fetch returned zero items. So a transiently-empty fetch (server hiccup, parser edge case) silently armed the firehose: the *next* non-empty fetch treated every backlog item as new. The first-fetch suppression existed precisely to prevent this. One-line guard on `!feed.items.is_empty()`. Please don't congratulate yourself for seeding a feed you haven't actually seen. Then: `fire_exec_on_new` ran `save_data()` once per feed arrival. With 50 bookmarks and the hook configured, that's 50 whole-file JSON writes per refresh. Split into `collect_exec_on_new` (in-memory only) and `flush_exec_on_new` (one save, then spawn); the receive loop accumulates argv across the batch and flushes once. AT-MOST-ONCE crash semantics are preserved -- save still happens before any spawn, just at batch granularity now. Finally: `remove_current_feed` never dropped the URL from `feeds_seeded` or its item IDs from `seen_items`. So the persisted JSON grew monotonically across feed churn for absolutely no reason. Drop them before removing the feed itself. Three regression tests added: dashboard filter reapply after macro toggle-read, empty-first-fetch suppression, and feed removal cleanup. 118 tests pass, clippy clean, fmt clean. --- src/app.rs | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++- src/events.rs | 42 ++++++++++++++++++++++++++ src/tui.rs | 63 +++++++++++++++++++++++++-------------- 3 files changed, 164 insertions(+), 23 deletions(-) diff --git a/src/app.rs b/src/app.rs index c222579..f862b29 100644 --- a/src/app.rs +++ b/src/app.rs @@ -903,6 +903,16 @@ impl App { if idx < self.feeds.len() { let url = self.feeds[idx].url.clone(); + // Drop this feed's exec_on_new tracking state before removing + // the feed itself — otherwise the URL would stay in + // feeds_seeded and its item IDs would stay in seen_items + // forever, growing the persisted JSON file monotonically. + self.feeds_seeded.remove(&url); + for item in &self.feeds[idx].items { + let id = make_item_id(&self.feeds[idx], item); + self.seen_items.remove(&id); + } + // Remove from feeds self.feeds.remove(idx); @@ -1726,7 +1736,12 @@ pub fn mark_feed_seen(app: &mut App, feed: &Feed) -> Vec { newly_seen.push(idx); } } - if is_first_fetch { + // Only flip the seeded bit on a fetch that actually returned items. A + // transiently-empty first fetch (server hiccup, parser edge case) would + // otherwise mark the feed as seeded with no items recorded; the next + // non-empty fetch would then treat every item as new and firehose + // exec_on_new for the entire backlog. + if is_first_fetch && !feed.items.is_empty() { app.feeds_seeded.insert(feed.url.clone()); } newly_seen @@ -2434,6 +2449,71 @@ mod tests { assert!(app.feeds_seeded.contains("https://ex.com/b.xml")); } + #[test] + fn test_mark_feed_seen_empty_first_fetch_does_not_seed() { + // Regression: a transiently-empty first fetch must not flip + // feeds_seeded. If it did, the next non-empty fetch would be treated + // as "subsequent" and fire exec_on_new for every backlog item — the + // exact firehose the first-fetch suppression exists to prevent. + let mut app = App::new(); + let empty = synthetic_feed("https://ex.com/feed.xml", &[]); + + let newly = mark_feed_seen(&mut app, &empty); + assert!(newly.is_empty()); + assert!( + !app.feeds_seeded.contains("https://ex.com/feed.xml"), + "empty fetch must NOT mark feed as seeded" + ); + + // The next fetch — now with items — is still the first real fetch + // and must seed silently. + let with_items = synthetic_feed("https://ex.com/feed.xml", &["A", "B", "C"]); + let newly = mark_feed_seen(&mut app, &with_items); + assert!( + newly.is_empty(), + "first non-empty fetch must seed silently, not fire" + ); + assert!(app.feeds_seeded.contains("https://ex.com/feed.xml")); + assert_eq!(app.seen_items.len(), 3); + } + + #[test] + fn test_remove_current_feed_cleans_up_tracking_state() { + // Regression: feed removal used to leave the URL in feeds_seeded and + // every item ID in seen_items forever, growing the persisted JSON + // file monotonically across feed churn. + let mut app = App::new(); + let feed = synthetic_feed("https://ex.com/feed.xml", &["A", "B"]); + let item_ids: Vec = feed.items.iter().map(|i| make_item_id(&feed, i)).collect(); + + // Seed the tracking state as if exec_on_new had run for this feed. + mark_feed_seen(&mut app, &feed); + let second = synthetic_feed("https://ex.com/feed.xml", &["A", "B", "C"]); + mark_feed_seen(&mut app, &second); + assert!(app.feeds_seeded.contains("https://ex.com/feed.xml")); + for id in &item_ids { + assert!(app.seen_items.contains(id), "precondition: {} seen", id); + } + + // Install the feed and trigger removal via the public path. + app.bookmarks.push("https://ex.com/feed.xml".to_string()); + app.feeds.push(second); + app.selected_feed = Some(0); + app.remove_current_feed().unwrap(); + + assert!( + !app.feeds_seeded.contains("https://ex.com/feed.xml"), + "URL must be dropped from feeds_seeded" + ); + for id in &item_ids { + assert!( + !app.seen_items.contains(id), + "item id {} must be dropped from seen_items", + id + ); + } + } + #[test] fn test_make_item_id_prefers_link() { let mut feed = Feed { diff --git a/src/events.rs b/src/events.rs index d6dbc0b..c883cbc 100644 --- a/src/events.rs +++ b/src/events.rs @@ -88,6 +88,11 @@ fn handle_toggle_read_current(app: &mut App) { app.error = Some(format!("Failed to toggle read status: {}", e)); } } + // Re-filter so a `read_status = Some(false)` ("unread only") filter on the + // Dashboard drops the just-read item from the visible list. The Dashboard + // keypress used to do this inline; centralizing it here means macros and + // every other caller stay in sync. + app.apply_filters(); } // ── Macro engine ─────────────────────────────────────────────────── @@ -2067,4 +2072,41 @@ mod tests { ); assert!(app.error.is_none(), "should not error: {:?}", app.error); } + + #[test] + fn test_dispatch_toggle_read_reapplies_filters_on_dashboard() { + // Regression: a `,r = toggle-read` macro on the Dashboard with the + // "unread only" filter active must remove the just-read item from the + // visible list. Previously dispatch_action -> handle_toggle_read_current + // skipped apply_filters(), so the dashboard kept showing stale state + // until the next filter change. + let mut app = make_test_app(); + app.read_items.clear(); + app.starred_items.clear(); + app.view = View::Dashboard; + + // Activate "unread only" filter, then materialize the filtered list. + app.filter_options.read_status = Some(false); + app.apply_filters(); + let before = app.active_dashboard_items().len(); + assert!( + before > 0, + "precondition: dashboard has unread items to filter on" + ); + app.selected_item = Some(0); + + dispatch_action(&mut app, KeyAction::ToggleRead); + + assert_eq!( + app.read_items.len(), + 1, + "item should be marked read via dispatch_action" + ); + assert_eq!( + app.active_dashboard_items().len(), + before - 1, + "the just-read item must drop out of the unread-only filter" + ); + assert!(app.error.is_none(), "should not error: {:?}", app.error); + } } diff --git a/src/tui.rs b/src/tui.rs index 7ba20f6..a3e43a8 100644 --- a/src/tui.rs +++ b/src/tui.rs @@ -201,40 +201,50 @@ fn drain_macro_steps(terminal: &mut Terminal, app: &mut App) { } } -/// Diff a freshly-arrived feed against `app.seen_items` and fire the -/// `[hooks].exec_on_new` template (if configured) for each genuinely-new item. -/// On the first successful fetch of a feed (URL not yet in `feeds_seeded`), -/// seed the seen set silently to suppress the firehose. +/// Diff a freshly-arrived feed against `app.seen_items`, mutate the seen +/// state in memory, and return the argv list to spawn for each genuinely-new +/// item. On the first successful fetch of a feed (URL not yet in +/// `feeds_seeded`), seed the seen set silently and return an empty list. /// -/// Persistence order is "mark-seen, persist, spawn" so semantics on crash are -/// AT MOST ONCE: a kill between persist and spawn loses a notification, but a -/// later restart never re-fires the same item. This matters for side-effecting -/// hooks like `wallabag-cli add`. +/// Returns Vec rather than spawning + saving inline so a refresh batch can +/// `save_data` ONCE before spawning ANY child, instead of once per feed +/// arrival (write amplification of N for N bookmarks). Crash semantics still +/// AT-MOST-ONCE: see [`flush_exec_on_new`] for the persistence ordering. /// /// No-op when the hook is not configured. seen_items / feeds_seeded only /// exist to drive this hook, so users who never opt in pay neither the -/// memory cost nor the per-feed save_data round-trip. -fn fire_exec_on_new(app: &mut App, feed: &Feed) { +/// memory cost nor any save_data round-trip. +fn collect_exec_on_new(app: &mut App, feed: &Feed) -> Vec> { let argv_template = match app.exec_on_new_template.as_ref() { Some(t) => t.clone(), - None => return, + None => return Vec::new(), }; let newly_seen_idx = crate::app::mark_feed_seen(app, feed); if newly_seen_idx.is_empty() { + return Vec::new(); + } + newly_seen_idx + .into_iter() + .filter_map(|idx| { + let item = feed.items.get(idx)?; + let ctx = crate::app::article_context_from(feed, item); + Some(crate::app::expand_argv_template(&argv_template, &ctx)) + }) + .collect() +} + +/// Persist the updated seen-set, then spawn all collected exec_on_new +/// children. The "save then spawn" order is the AT-MOST-ONCE guarantee: +/// a kill between save and spawn loses notifications, but a restart never +/// re-fires the same item. Matters for side-effecting hooks like +/// `wallabag-cli add` where a duplicate is actively wrong. +fn flush_exec_on_new(app: &mut App, pending: Vec>) { + if pending.is_empty() { return; } - // Persist the updated seen set before spawning. If we crash before - // spawning, the user misses a notification — preferable to a duplicate - // for side-effecting hooks. let _ = app.save_data(); let mut first_failure_recorded = false; - for idx in newly_seen_idx { - let item = match feed.items.get(idx) { - Some(i) => i, - None => continue, - }; - let ctx = crate::app::article_context_from(feed, item); - let argv = crate::app::expand_argv_template(&argv_template, &ctx); + for argv in pending { if let Err(e) = spawn_detached(&argv) { if !first_failure_recorded { app.error = Some(format!("exec_on_new: {}", e)); @@ -306,9 +316,14 @@ fn run_app(terminal: &mut Terminal, app: &mut App) -> Result<()> // Drain any feeds that arrived from background threads if pending_count > 0 { + // Accumulate exec_on_new spawns across every feed arriving in this + // tick, so we only `save_data` once before spawning. Saving per + // feed (the previous shape) was N whole-file JSON writes per + // refresh. + let mut pending_exec: Vec> = Vec::new(); while let Ok((idx, result)) = feed_rx.try_recv() { if let Ok(feed) = result { - fire_exec_on_new(app, &feed); + pending_exec.extend(collect_exec_on_new(app, &feed)); // Insert at the correct position to maintain bookmark order, // or append if earlier feeds haven't arrived yet let insert_pos = app @@ -349,6 +364,10 @@ fn run_app(terminal: &mut Terminal, app: &mut App) -> Result<()> let _ = app.save_data(); } } + // Persist seen-set ONCE for the whole batch, then spawn. Done + // outside the per-feed loop so a refresh that brings back items + // for many feeds writes the JSON file once instead of N times. + flush_exec_on_new(app, pending_exec); } // If loading, use a shorter timeout for animation From 515a53f0f69851b43fd085361157b4dd2527f445 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Wed, 13 May 2026 22:56:24 +0200 Subject: [PATCH 6/6] docs: document external-command hooks (macros, exec_on_new) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five commits of feature work landed on this branch and the docs never caught up. Anyone reading the README would have no idea feedr now does newsboat-style macros, pipe-to, exec, or exec_on_new notifications. CLAUDE.md still described the codebase as if none of this existed. Let's fix that. README gets a Features bullet plus a dedicated subsection covering the no-shell argv expansion model, the %X template variables, macro chain syntax + step kinds, exec_on_new with its at-most-once crash semantics, and the security footguns. The `sh -c "... %t..."` warning is worth repeating outside of the in-code config comments — that is the one mistake that turns a safe feature into shell injection through item titles, and I would rather say it twice than once. CLAUDE.md gets two new "Key patterns" bullets explaining the engine architecture future-me will want: queue-fill in events.rs, drain in tui.rs because pipe-to needs the terminal handle to suspend the TUI; save-then-spawn ordering for at-most-once semantics; first-fetch seeding to prevent the firehose; transiently-empty-fetch guard so we do not seed prematurely. Module descriptions updated to mention the new responsibilities. Not glamorous, but the feature is not shipped until the docs say it is. --- CLAUDE.md | 14 +++++++------ README.md | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 6 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 3c0c1c0..ab71c5f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -4,7 +4,7 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co ## Project Overview -Feedr is a terminal-based RSS/Atom feed reader built with Rust, using ratatui/crossterm for the TUI. It supports feed management, categorization, filtering, dual themes, OPML import, auto-refresh with per-domain rate limiting, feed auto-discovery from HTML pages, configurable keybindings, mouse support, and a help overlay. +Feedr is a terminal-based RSS/Atom feed reader built with Rust, using ratatui/crossterm for the TUI. It supports feed management, categorization, filtering, dual themes, OPML import, auto-refresh with per-domain rate limiting, feed auto-discovery from HTML pages, configurable keybindings, mouse support, a help overlay, and newsboat-style external-command hooks (macros and `exec_on_new`). ## Build & Development Commands @@ -28,11 +28,11 @@ MSRV: 1.75.0. CI runs tests on stable, beta, and 1.75.0. ### Core modules - **`app.rs`** — `App` struct holding all application state. All state mutations (feed ops, filtering, categorization, persistence) happen through its methods. This is the largest and most central file. -- **`tui.rs`** — Terminal setup/teardown, main event loop (`run_app`), and feed refresh logic. -- **`events.rs`** — All keyboard and mouse event handling (`handle_events`). Input dispatches based on `View` × `InputMode` enums. Separated from `tui.rs` for maintainability. -- **`keybindings.rs`** — `KeyAction` enum, default keybinding map, key string parsing, and config-driven keybinding overrides via `[keybindings]` TOML section. +- **`tui.rs`** — Terminal setup/teardown, main event loop (`run_app`), feed refresh logic, and external-command runners (`suspend_for_command`, `spawn_detached`, `drain_macro_steps`, `collect_exec_on_new`/`flush_exec_on_new`). `TerminalRestoreGuard` (RAII) re-enters alt-screen + raw mode + mouse capture on drop so a panic in a child invocation can't leave the terminal broken. +- **`events.rs`** — All keyboard and mouse event handling (`handle_events`). Input dispatches based on `View` × `InputMode` enums. Hosts the macro engine (`dispatch_action`, `run_macro`, `build_pipe_invocation`, `build_exec_invocation`). Separated from `tui.rs` for maintainability. +- **`keybindings.rs`** — `KeyAction` enum, default keybinding map, key string parsing, config-driven keybinding overrides via `[keybindings]` TOML section, and newsboat-style macro parsing (`MacroStep`, `MacroBinding`, `MacroOptions`, `parse_macro_string`). - **`feed.rs`** — Data models (`Feed`, `FeedItem`, `FeedCategory`), RSS/Atom parsing via `feed-rs`, and HTML feed auto-discovery via `scraper`. -- **`config.rs`** — XDG-compliant config loading/saving (`~/.config/feedr/config.toml`). Includes `keybindings: HashMap` for custom key overrides. Auto-generates defaults on first run. +- **`config.rs`** — XDG-compliant config loading/saving (`~/.config/feedr/config.toml`). Includes `keybindings: HashMap` for custom key overrides, `[hooks]` (`exec_on_new`), `[macros]`, and `[macro_options]`. Auto-generates defaults on first run. - **`config_cli.rs`** — CLI subcommand handler for `feedr config list/get/set`. - **`config_tui.rs`** — Interactive TUI config editor (`feedr config --tui`). - **`config_ui.rs`** — Rendering for the TUI config editor. @@ -64,6 +64,8 @@ MSRV: 1.75.0. CI runs tests on stable, beta, and 1.75.0. - **Rate limiting**: `last_domain_fetch: HashMap` throttles per-domain HTTP requests. - **Authenticated feeds**: `feed_headers: HashMap>` in `App` maps feed URLs to custom HTTP headers. Built from `config.default_feeds` entries that have `headers`. Passed to `Feed::fetch_url()` at all fetch call sites. - **Compact mode**: `app.compact` bool is updated each frame by `update_compact_mode(terminal_height)`. Rendering in `ui.rs` branches on `app.compact` for layout, title bar, help bar, and dashboard item format. Controlled by `config.ui.compact_mode` (`Auto`/`Always`/`Never`). Dialog modals use `centered_rect_with_min()` to enforce minimum dimensions regardless of compact mode. +- **External-command hooks (macros + `exec_on_new`)**: Commands are run **without a shell**. Templates are tokenized once at config load via `shlex`, then `expand_argv_template` substitutes `%X` placeholders into individual argv tokens (no re-expansion), so feed content cannot break out of an argument. The macro engine queues steps into `app.pending_macro_steps` from `events.rs` and the TUI loop drains them in `tui.rs::drain_macro_steps` — drain lives at the loop level because `pipe-to` needs the terminal handle to suspend the TUI. Chains halt on the first step error (tracked via a `pre_error` guard so a stale `app.error` doesn't spuriously abort). The macro prefix (default `,`) is checked at the top of `handle_key_event` and only when `input_mode == Normal`, so text-input modes are not disturbed; an idle prefix times out via the existing success-message timeout. +- **`exec_on_new` crash semantics**: AT-MOST-ONCE. `flush_exec_on_new` persists the `seen_items` / `feeds_seeded` sets *before* spawning any child, so a kill mid-fire loses a notification rather than re-firing on the next launch. `mark_feed_seen` only flips `feeds_seeded` on a fetch that returned items (transiently-empty first fetches don't seed), and the first observation of a feed seeds the seen-set silently to avoid a firehose. Children are spawned detached with stdio nulled; a reaper thread waits on each so they don't linger as zombies. The seen-set is pruned in `remove_current_feed` to prevent monotonic growth across feed churn. ## Commit Conventions @@ -71,4 +73,4 @@ Uses **conventional commits** — `feat:`, `fix:`, `refactor:`, `docs:`, `perf:` ## Testing -Integration tests live in `/tests/integration_test.rs` and test feed parsing against real URLs. Unit tests are inline in `config.rs`, `app.rs`, and `keybindings.rs`. +Integration tests live in `/tests/integration_test.rs` and test feed parsing against real URLs. Unit tests are inline in `config.rs`, `app.rs`, `keybindings.rs`, `events.rs`, and `tui.rs` (the macro-drain tests use a `TestBackend` to avoid needing a real TTY). diff --git a/README.md b/README.md index 9a72e30..c29c125 100644 --- a/README.md +++ b/README.md @@ -36,6 +36,7 @@ Feedr is a feature-rich terminal-based RSS feed reader written in Rust. It provi - **Compact Mode**: Automatic compact layout for small terminals (≤30 rows), with manual `always`/`never` override in config - **CLI Config Management**: Get, set, and list configuration from the command line (`feedr config`), or use the interactive TUI config editor (`feedr config --tui`) - **Configurable Keybindings**: Remap any key action via the `[keybindings]` section in `config.toml` +- **External-Command Hooks**: Newsboat-style macros (`pipe-to`, `exec`) bound to keys, plus `exec_on_new` notifications fired per new item — all with shell-free argument templating - **Configurable**: Customize timeouts, themes, UI behavior, and default feeds via TOML config - **XDG Compliant**: Follows standard directory specifications for configuration and data storage @@ -313,6 +314,66 @@ Cookie = "session=abc123" ``` Headers are sent with every request for that feed, including refreshes. +### External-Command Hooks + +Feedr supports newsboat-style external commands for two workflows: **macros** (key-triggered chains that act on the focused article) and **`exec_on_new`** (a notification hook fired per newly-seen item after each refresh). + +**Commands are not run through a shell.** Templates are tokenized once at config load, and `%X` placeholders are substituted into individual `argv` tokens — feed content can never break out of an argument. For pipes, redirection, or globbing, write a small shell script and invoke that. + +#### Template Variables + +Expanded in every `argv` token of macro and hook commands: + +| Variable | Expands to | +|----------|------------| +| `%t` | Article title | +| `%u` | Article URL | +| `%a` | Author | +| `%d` | Formatted publish date | +| `%f` | Feed title | +| `%F` | Feed URL | +| `%%` | Literal `%` | + +#### Macros + +A macro binds a key to an ordered chain of steps. Trigger with `` (default prefix is `,`). Steps are separated by `;`. An optional trailing ` -- "description"` overrides the help-overlay label. + +```toml +[macros] +y = 'open-in-browser ; pipe-to "yt-dlp %u"' +w = 'pipe-to "wallabag-cli add %u" -- "Save to Wallabag"' +n = 'pipe-to "tee /tmp/out.txt" stdin=metadata' + +[macro_options] +prefix = "," # the macro-prefix key +pipe_default_stdin = "body" # body | title | url | metadata | none +``` + +**Step kinds:** +- `` — invoke a built-in action. Supported in macros: `open-in-browser`, `toggle-star`, `toggle-read`, `mark-all-read`, `refresh`, `toggle-theme`, `extract-links`, `help`. +- `pipe-to "cmd %u" [stdin=…]` — suspend the TUI, run the command, and pipe article content to its stdin. `stdin` is one of `body` (default), `title`, `url`, `metadata`, or `none`. +- `exec "cmd %u"` — spawn the command detached (no stdin, no terminal takeover). + +Chains halt on the first step error. Press `Esc` after the prefix to cancel; an unbound follow-up surfaces a "No macro bound" error. Macros are also rendered in the help overlay (`?`). + +#### `exec_on_new` Notifications + +Fire a command once per newly-seen item after each refresh. The first successful fetch of each feed seeds the seen-set silently — you do **not** get a firehose on initial load or first run. + +```toml +[hooks] +exec_on_new = 'notify-send "New: %t" "%f"' +``` + +Children are spawned **detached** so the TUI never blocks on them. Crash semantics are **at-most-once**: feedr persists the seen-set before spawning, so a kill mid-fire loses a notification rather than re-firing on the next launch. Prefer idempotent commands (e.g. `wallabag-cli add` is safe; `mail-me` is not). + +#### Security Notes + +- The shell is never invoked, so feed content in `%t` / `%a` / etc. cannot escape an argument. +- **Do not wrap your command in `sh -c "... %t ..."`** — that reintroduces shell injection through item titles. Write a script file and invoke it instead. +- `~` / `$HOME` / `$VAR` are **not** expanded — use absolute paths. +- If a macro's command template has unbalanced quotes or names an unknown action, feedr surfaces a startup warning rather than failing silently at trigger time. + ### Configurable Keybindings Remap any action by adding a `[keybindings]` section to your config file. Each action can be bound to a single key string or an array of keys: