fix(tui): address review feedback on unread feature

CodeRabbit review on agent-of-empires#2088:

- Promote unread in `attention_group_key` too, so project-grouped Attention
  agrees with the flat view (a group with an unread Idle now outranks a group
  whose best member is a read Error).
- Map `v`/`V` to `ToggleUnread` in the context menu's `handle_key`, with a
  regression test; the quick-pick only fires when the row is present.
- Gate the `v` binding behind the feature: a new `Context::UnreadEnabled`
  drops it from dispatch when off, and the help overlay and command palette
  skip it, so disabling the setting removes it from every surface (not just a
  no-op handler).
- Clear auto-unread unconditionally (only when an `Auto` flag is present) so a
  stale marker can't survive a disable/re-enable; `Manual` is still preserved.
- Fall back an omitted theme `unread` to that theme's own `accent` instead of
  Empire's default, via a load-time `fill_unread_from_accent`.
- Lighten catppuccin-latte's `unread` so it stays below Waiting in that light
  theme (waiting > unread > idle).

🤖 Generated with Claude Code

Author: Eric162

src/session/groups.rs

@@ -686,9 +686,15 @@ fn attention_group_key(
     // contribute; the real tiebreak is the trailing ASC field. Shape
     // mirrors `attention_session_key` so the intent is uniform.
     let max_last = members.iter().filter_map(|i| i.last_accessed_at).max();
+    // Fold the unread promoter in at the group level too, so a project
+    // containing an unread session floats up just like the flat Attention
+    // view does (a group with an unread Idle outranks a group whose best
+    // member is a read Error). Mirrors `attention_session_key`'s rank.
+    let unread = members.iter().any(|i| i.is_unread());
+    let rank = attention_rank(min_tier, unread, crate::session::unread_enabled());
     (
         !urgent_bias,
-        min_tier,
+        rank,
         !favorite_bias,
         max_last.is_none(),
         Reverse(None),
@@ -1924,9 +1930,11 @@ mod tests {
 
     #[test]
     fn test_attention_group_key_one_active_pulls_group_up() {
-        // Mixed group: 1 archived Waiting, 1 active Idle. Group should sort
-        // at tier 2 (Idle); the active member pulls it out of the archive
-        // tier. This is the auto-unarchive contract for cascade.
+        // Mixed group: 1 archived Waiting, 1 active Idle. The active member
+        // pulls the group out of the archive tier (99) into the Idle bucket.
+        // With the unread promoter enabled (default), a non-unread Idle group
+        // encodes to rank 3 (Waiting=0, unread=1, other tiers shifted +1, so
+        // Idle tier 2 -> 3); the point is it's well above the archive tier.
         let mut archived_waiting = Instance::new("aw", "/tmp/aw");
         archived_waiting.group_path = "work".to_string();
         archived_waiting.status = crate::session::Status::Waiting;
@@ -1938,8 +1946,31 @@ mod tests {
         let instances = vec![archived_waiting, active_idle];
         let key = attention_group_key("work", None, &instances);
         assert_eq!(
-            key.1, 2,
-            "group with one active Idle session should sort at tier 2"
+            key.1, 3,
+            "active Idle group should sort in the Idle bucket, far above archive tier 99"
+        );
+    }
+
+    #[test]
+    fn test_attention_group_key_promotes_unread_below_waiting() {
+        // Group A holds a read Error (tier 1); Group B holds an unread Idle
+        // (tier 2). With the unread promoter on (default), B's unread member
+        // floats it to rank 1, above A's Error (rank 2), matching how the flat
+        // Attention view promotes unread just below Waiting.
+        let mut err = Instance::new("err", "/tmp/err");
+        err.group_path = "a".to_string();
+        err.status = crate::session::Status::Error;
+
+        let mut unread_idle = Instance::new("ui", "/tmp/ui");
+        unread_idle.group_path = "b".to_string();
+        unread_idle.status = crate::session::Status::Idle;
+        unread_idle.mark_unread_auto();
+
+        let key_a = attention_group_key("a", None, std::slice::from_ref(&err));
+        let key_b = attention_group_key("b", None, std::slice::from_ref(&unread_idle));
+        assert!(
+            key_b < key_a,
+            "group with an unread Idle must outrank a group whose best member is a read Error: b={key_b:?} a={key_a:?}"
         );
     }
 

src/tui/components/help.rs

@@ -50,6 +50,13 @@ fn shortcuts(strict: bool, live_on_enter: bool) -> Vec<(&'static str, Vec<(Strin
     let mut other: Vec<(String, String)> = Vec::new();
     for b in bindings::BINDINGS {
         let Some(help) = &b.help else { continue };
+        // The unread toggle is fully removed when the feature is off, so the
+        // help overlay shouldn't advertise a dead key (unlike the sort-gated
+        // Attention rows, which stay listed because that's a transient view
+        // state, not a disabled feature).
+        if b.id == bindings::ActionId::ToggleUnread && !crate::session::unread_enabled() {
+            continue;
+        }
         let mut label = bindings::label(b.id, strict);
         if label.is_empty() {
             label = bindings::label(b.id, false);

src/tui/dialogs/command_palette.rs

@@ -94,6 +94,11 @@ pub fn builtin_commands(serve_enabled: bool, strict_hotkeys: bool) -> Vec<Palett
             if meta.serve_only && !serve_enabled {
                 return None;
             }
+            // Drop the unread toggle entirely when the feature is off, so the
+            // palette can't invoke a removed binding.
+            if b.id == bindings::ActionId::ToggleUnread && !crate::session::unread_enabled() {
+                return None;
+            }
             Some(PaletteCommand {
                 id: bindings::palette_id(b.id),
                 title: meta.title.to_string(),

src/tui/dialogs/context_menu.rs

@@ -268,6 +268,7 @@ impl ContextMenuDialog {
                     'd' | 'D' => Some(ContextMenuAction::Delete),
                     'z' | 'Z' => Some(ContextMenuAction::ToggleArchive),
                     'h' | 'H' => Some(ContextMenuAction::ToggleSnooze),
+                    'v' | 'V' => Some(ContextMenuAction::ToggleUnread),
                     // `n` opens a new session from whichever new-session entry
                     // the current menu carries: the group/project menu prefills
                     // from the row (NewFromGroup), the empty-sidebar menu opens
@@ -505,6 +506,23 @@ mod tests {
         assert_eq!(labels, vec!["Rename", "Archive", "Delete"]);
     }
 
+    #[test]
+    fn v_hotkey_submits_toggle_unread() {
+        // The `v` quick-pick mirrors the home-view shortcut and only fires
+        // when the Unread row is present (feature enabled).
+        let mut menu = ContextMenuDialog::for_session((0, 0), false, None, Some(false));
+        assert!(matches!(
+            menu.handle_key(key(KeyCode::Char('v'))),
+            DialogResult::Submit(ContextMenuAction::ToggleUnread)
+        ));
+        // With no Unread row (feature off), `v` is inert.
+        let mut off = ContextMenuDialog::for_session((0, 0), false, None, None);
+        assert!(matches!(
+            off.handle_key(key(KeyCode::Char('v'))),
+            DialogResult::Continue
+        ));
+    }
+
     #[test]
     fn h_hotkey_submits_toggle_snooze() {
         let mut menu = ContextMenuDialog::for_session((0, 0), false, Some(false), None);

src/tui/home/bindings.rs

@@ -111,6 +111,10 @@ pub enum Context {
     SearchActive,
     /// The cursor is on a real (non-synthetic) project header in project view.
     ProjectGroupSelected,
+    /// The unread-session feature is enabled (`session.unread_indicator`). When
+    /// off, the binding is removed from dispatch so the key isn't swallowed by
+    /// a dead action; help and the command palette skip it separately.
+    UnreadEnabled,
 }
 
 /// Help-overlay section. Ordering mirrors `components/help.rs`.
@@ -169,6 +173,7 @@ fn context_holds(context: Context, ctx: &Ctx) -> bool {
         Context::AttentionSort => ctx.sort_order == SortOrder::Attention,
         Context::SearchActive => ctx.has_search,
         Context::ProjectGroupSelected => ctx.project_group_selected,
+        Context::UnreadEnabled => crate::session::unread_enabled(),
     }
 }
 
@@ -610,7 +615,7 @@ pub static BINDINGS: &[Binding] = &[
         id: ActionId::ToggleUnread,
         non_strict: &[k('v')],
         strict: &[k('V')],
-        context: Context::Always,
+        context: Context::UnreadEnabled,
         help: Some(HelpMeta {
             section: HelpSection::Actions,
             desc: "Mark read/unread (toggle)",

src/tui/home/mod.rs

@@ -4858,11 +4858,16 @@ impl HomeView {
     }
 
     /// Clear an auto-unread marker because the user viewed the session
-    /// (Tab into live-send, Enter to attach). No-op when the unread feature
-    /// is disabled; a manual flag is left in place (it clears only via the
-    /// manual toggle). Persists via `apply_user_action`.
+    /// (Tab into live-send, Enter to attach). A manual flag is left in place
+    /// (it clears only via the manual toggle). Runs regardless of the feature
+    /// flag so a stale `Auto` marker can't survive a disable/re-enable and
+    /// reappear later; only writes when there's actually an `Auto` flag to
+    /// clear, so a viewed read session doesn't churn the storage flock.
     pub(crate) fn clear_auto_unread(&mut self, id: &str) {
-        if crate::session::unread_enabled() {
+        let has_auto = self
+            .get_instance(id)
+            .is_some_and(|i| i.unread == Some(crate::session::UnreadKind::Auto));
+        if has_auto {
             let _ = self.apply_user_action(id, |i| i.mark_read_auto());
         }
     }

src/tui/styles/mod.rs

@@ -131,21 +131,41 @@ fn load_custom_theme(path: &std::path::Path) -> Option<Theme> {
     };
 
     match toml::from_str::<Theme>(&content) {
-        Ok(theme) => Some(theme),
+        Ok(theme) => Some(fill_unread_from_accent(&content, theme)),
         Err(e) => {
             warn!("Failed to parse theme file {}: {}", path.display(), e);
             None
         }
     }
 }
 
+/// A theme TOML that omits `unread` should inherit that theme's own `accent`,
+/// not Empire's default blue. The container `#[serde(default)]` seeds every
+/// omitted field from `Theme::default()` (= Empire), and serde can't tell an
+/// omitted key from one explicitly set to Empire's value, so we detect the
+/// omission from the raw table and fall back to the parsed theme's accent.
+fn fill_unread_from_accent(content: &str, mut theme: Theme) -> Theme {
+    let omitted = content
+        .parse::<toml::Table>()
+        .map(|t| !t.contains_key("unread"))
+        .unwrap_or(false);
+    if omitted {
+        theme.unread = theme.accent;
+    }
+    theme
+}
+
 /// Parse a builtin's embedded TOML. Builtin TOMLs are committed to the repo
 /// and embedded at build time; a parse failure here is a developer bug, not
 /// user input. The `all_builtins_parse_with_expected_anchors` test guards
 /// against that landing in main.
 fn parse_builtin(builtin: &BuiltinTheme) -> Theme {
-    toml::from_str(builtin.source)
-        .unwrap_or_else(|e| panic!("builtin theme '{}' failed to parse: {}", builtin.name, e))
+    let theme = toml::from_str(builtin.source)
+        .unwrap_or_else(|e| panic!("builtin theme '{}' failed to parse: {}", builtin.name, e));
+    // All builtins define `unread`, so this is a no-op for them today, but
+    // keep the same fallback as custom themes so a future builtin that omits
+    // it inherits its own accent rather than Empire's.
+    fill_unread_from_accent(builtin.source, theme)
 }
 
 pub fn load_theme(name: &str) -> Theme {

src/tui/styles/themes.rs

@@ -90,8 +90,9 @@ pub struct Theme {
     /// user hasn't viewed, or a manual "flag for later"). Applied to resting
     /// rows (Idle/Unknown) in place of the decaying idle color so unread work
     /// stands out without being as loud as Waiting/Error. Gated behind the
-    /// `session.unread_indicator` config toggle (on by default). Defaults to
-    /// the theme's accent if a custom TOML omits it.
+    /// `session.unread_indicator` config toggle (on by default). A theme TOML
+    /// that omits this key inherits that theme's own `accent` (filled at load
+    /// time by `fill_unread_from_accent`), not Empire's default.
     #[serde(with = "hex_color")]
     pub unread: Color,
     #[serde(with = "hex_color")]

themes/builtin/catppuccin-latte.toml

@@ -16,7 +16,7 @@ running = "#40a02b"
 waiting = "#fe640b"
 fresh_idle = "#df8e1d"
 idle = "#9ca0b0"
-unread = "#04a5e5"
+unread = "#7287fd"
 error = "#d20f39"
 terminal_active = "#1e66f5"
 group = "#179299"