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.

Author: bahdotsh

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<usize> {
             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<String> = 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 {

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);
+    }
 }

src/tui.rs

@@ -201,40 +201,50 @@ fn drain_macro_steps<B: Backend>(terminal: &mut Terminal<B>, 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<Vec<String>> {
     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<Vec<String>>) {
+    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<B: Backend>(terminal: &mut Terminal<B>, 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<String>> = 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<B: Backend>(terminal: &mut Terminal<B>, 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