feat: unread session state (TUI + web dashboard)#2088
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces a complete session unread marking feature across TUI and web clients. Sessions can be marked unread via Auto (on ChangesUnread Session Marking Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes The PR spans multiple subsystems with careful state management (Auto vs Manual unread semantics with non-clobbering rules), sorting integration via pure functions, comprehensive theming with accent fallback logic, keybinding wiring through multiple TUI dispatch layers, context menu updates with dynamic labels, server-side status polling auto-marking with profile-scoped batching, REST API integration with read-only guards, and web client optimistic state reconciliation spanning sidebar triage overlay, prop threading, render logic, and error handling. While individual pieces are straightforward, the feature's spread across config, instance model, sorting, theme, TUI input dispatch, rendering, help system, server polling, API endpoint, and web client state management requires understanding cross-module interactions and subtle behavioral details like auto-marking suppression during active sends and optimistic reconciliation with server state. The test coverage is thorough throughout both TUI and web layers, helping clarify intent. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
|
Woah. I love this idea. I find myself in the same situation, I forget which idle sessions i looked at vs didn't look at! Let me think about the best way to fit this into the design. I'm going back and forth about how we best introduce this as an optional feature or should we even just have it as the default... I think the env var is too conservative, i don't want to make it that tricky for someone to use it. |
Contributor
Author
|
@njbrake sounds great - let me know what you're thinking on how to intro it, and i can update it. Maybe Default + an off switch for those who don't like it? |
Eric162
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tui/home/mod.rs (1)
5398-5444:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRebuild the cached list when
unread_indicatorflips.This updates the process-wide unread gate, but the cached
flat_itemsordering is left as-is. Becauseswitch_profile()doesreload()beforerefresh_from_config(), switching between profiles with differentsession.unread_indicatorvalues can leave Attention sort using the previous unread ranking until some unrelated rebuild happens. Recompute the derived list state when this flag changes.Suggested fix
fn apply_config_to_state( &mut self, config: crate::session::Config, origin: ConfigRefreshOrigin, ) { + let unread_changed = crate::session::unread_enabled() != config.session.unread_indicator; self.default_terminal_mode = match config.sandbox.default_terminal_mode { DefaultTerminalMode::Host => TerminalMode::Host, DefaultTerminalMode::Container => TerminalMode::Container, }; @@ self.idle_decay_window = crate::tui::styles::idle_decay_window(config.theme.idle_decay_minutes); crate::session::set_unread_enabled(config.session.unread_indicator); + if unread_changed { + self.flat_items = self.build_flat_items(); + if self.search_active && !self.search_query.value().is_empty() { + self.update_search(); + } else if !self.search_matches.is_empty() { + self.refresh_search_matches(); + } + self.update_selected(); + } self.tool_configs = config.tools;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tui/home/mod.rs` around lines 5398 - 5444, The code updates the process-wide unread flag via crate::session::set_unread_enabled but does not rebuild the cached ordering (flat_items), so flipping session.unread_indicator can leave Attention sort stale; modify apply_config_to_state to read the prior unread flag before calling crate::session::set_unread_enabled, call set_unread_enabled with config.session.unread_indicator, and if the value changed invoke the existing method that recomputes the derived list/cache (the function that rebuilds flat_items — e.g. self.rebuild_flat_items() or whatever method your codebase uses to refresh the cached list ordering) so the UI reflects the new unread ranking immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/session/groups.rs`:
- Around line 528-540: The grouped sorting key still uses the raw min_tier so
projects with unread lower-priority sessions aren't promoted; update the code
that builds the group key (function attention_group_key) to compute the tier
using the existing attention_rank(tier: u8, unread: bool, unread_enabled: bool)
helper instead of using min_tier directly (e.g., call attention_rank(min_tier,
has_unread, unread_enabled) when assembling the key), and make the same
replacement at the other occurrence referenced (around the second occurrence at
the 588-area) so grouped and flat Attention use the same unread-promoted
ranking.
In `@src/tui/dialogs/context_menu.rs`:
- Around line 22-25: The ContextMenuAction::ToggleUnread enum variant isn't
mapped in handle_key(), so pressing 'v'/'V' in the context menu does nothing;
update the handle_key() match (or key->action mapping) to treat 'v' and 'V' the
same as the existing ToggleUnread menu selection (i.e., return/contextually
trigger ContextMenuAction::ToggleUnread), and add a small regression test that
opens the context menu and asserts that sending 'v' selects/triggers
ToggleUnread (mirror tests for other actions to follow the same pattern).
In `@src/tui/home/bindings.rs`:
- Around line 59-62: The ToggleUnread binding is still present in the static
BINDINGS list so resolve() and UI help still claim the key even when
session.unread_indicator is disabled; remove the binding metadata instead of
relying on toggle_unread_at_cursor() no-op. Update the code that builds BINDINGS
(the constant/vec where ToggleUnread is declared) to conditionally include or
exclude the ToggleUnread entry based on the session config
(session.unread_indicator), and ensure any code paths that populate
help/command-palette entries use that same filtered list; also remove/avoid
matching 'v'/'V' in resolve() when the binding is not present so the key no
longer gets swallowed. Apply the same gating to the other occurrences referenced
around the 609-624 region.
In `@src/tui/home/mod.rs`:
- Around line 4860-4868: The guard in clear_auto_unread prevents clearing an
Auto unread flag when the unread indicator is disabled, allowing Auto to become
stale later; remove the conditional check and always call
self.apply_user_action(id, |i| i.mark_read_auto()) so auto-unread is cleared
unconditionally (mark_read_auto already preserves Manual). Update the function
clear_auto_unread to unconditionally invoke apply_user_action with
mark_read_auto, referencing clear_auto_unread, apply_user_action, and
mark_read_auto to locate the change.
In `@src/tui/styles/themes.rs`:
- Around line 89-96: The unread color should be deserialized as optional and
populated from the theme's accent after parsing: change Theme::unread from Color
to Option<Color> (preserve #[serde(with = "hex_color")] and add
#[serde(default)] if needed), then after deserializing a Theme (e.g. in the
theme-loading path or a Theme::finalize()/fill_defaults() method that runs
post-deserialize), set theme.unread =
theme.unread.or(Some(theme.accent.clone())); apply the same change for the other
mentioned fields (lines ~171-215 and ~233-244) so omitted fields inherit from
the parsed theme's values rather than Theme::default().
In `@themes/builtin/catppuccin-latte.toml`:
- Line 19: The unread color in themes/builtin/catppuccin-latte.toml (key unread)
is too dark compared to the Waiting tone and should be lightened so the intended
priority waiting > unread > idle is preserved; update the unread hex to a
slightly lighter blue (choose a hex lighter than the current `#04a5e5` and
visually less saturated than the Waiting orange) and confirm in
src/tui/home/render.rs that theme.unread will now render below Waiting but above
Idle/Unknown.
---
Outside diff comments:
In `@src/tui/home/mod.rs`:
- Around line 5398-5444: The code updates the process-wide unread flag via
crate::session::set_unread_enabled but does not rebuild the cached ordering
(flat_items), so flipping session.unread_indicator can leave Attention sort
stale; modify apply_config_to_state to read the prior unread flag before calling
crate::session::set_unread_enabled, call set_unread_enabled with
config.session.unread_indicator, and if the value changed invoke the existing
method that recomputes the derived list/cache (the function that rebuilds
flat_items — e.g. self.rebuild_flat_items() or whatever method your codebase
uses to refresh the cached list ordering) so the UI reflects the new unread
ranking immediately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b7a827e-08d3-42cf-9863-2bac81d93271
📒 Files selected for processing (21)
src/session/config.rssrc/session/groups.rssrc/session/instance.rssrc/session/mod.rssrc/tui/app.rssrc/tui/dialogs/context_menu.rssrc/tui/home/bindings.rssrc/tui/home/input.rssrc/tui/home/mod.rssrc/tui/home/operations.rssrc/tui/home/render.rssrc/tui/home/tests.rssrc/tui/styles/themes.rsthemes/builtin/catppuccin-latte.tomlthemes/builtin/deep-ocean.tomlthemes/builtin/dracula.tomlthemes/builtin/empire.tomlthemes/builtin/phosphor.tomlthemes/builtin/rose-pine.tomlthemes/builtin/tokyo-night-storm.tomlthemes/builtin/zinc.toml
Eric162
added a commit
to Eric162/agent-of-empires
that referenced
this pull request
Jun 15, 2026
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
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tui/home/mod.rs (1)
5421-5421:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRebuild Attention rows when the unread gate changes.
Line 5421 flips the process-wide unread flag, but
flat_itemscan still be ordered with the previous flag. In this file,switch_profile()reloads rows beforerefresh_from_config(), so changing profiles or togglingsession.unread_indicatorcan leave unread rows promoted after disabling, or not promoted after enabling, until a later rebuild. Please rebuild the list when this flag changes and Attention sort is active.🐛 Proposed fix
- crate::session::set_unread_enabled(config.session.unread_indicator); + let unread_was_enabled = crate::session::unread_enabled(); + crate::session::set_unread_enabled(config.session.unread_indicator); + let unread_enabled_changed = unread_was_enabled != config.session.unread_indicator; self.tool_configs = config.tools; self.tool_hotkey_cache = input::build_tool_hotkey_cache(&self.tool_configs); @@ if matches!(origin, ConfigRefreshOrigin::Interactive) && !hotkey_warnings.is_empty() && self.info_dialog.is_none() { self.info_dialog = Some(InfoDialog::new( "Tool hotkey config errors", &hotkey_warnings.join("\n"), )); } + if unread_enabled_changed && self.sort_order == SortOrder::Attention { + let selected_session = self.selected_session.clone(); + let selected_group = self.selected_group.clone(); + self.flat_items = self.build_flat_items(); + if let Some(pos) = selected_session + .as_deref() + .and_then(|session_id| { + self.flat_items.iter().position(|item| { + matches!(item, Item::Session { id, .. } if id == session_id) + }) + }) + .or_else(|| { + selected_group.as_deref().and_then(|group_path| { + self.flat_items.iter().position(|item| { + matches!(item, Item::Group { path, .. } if path == group_path) + }) + }) + }) + { + self.cursor = pos; + } else if self.cursor >= self.flat_items.len() { + self.cursor = self.flat_items.len().saturating_sub(1); + } + self.update_selected(); + } // Watcher path: stash for tick-loop dispatch (App owns theme state).This follows the PR objective that turning the setting off disables Attention-sort promotion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tui/home/mod.rs` at line 5421, The call to crate::session::set_unread_enabled() at line 5421 changes the process-wide unread flag but does not rebuild the flat_items list, causing the Attention sort ordering to become stale until a later rebuild. After calling set_unread_enabled(), check if the Attention sort is currently active and if so, trigger a rebuild of the item rows to ensure they are correctly ordered according to the new unread flag state. This ensures rows are immediately promoted or demoted from Attention-sort promotion based on whether the unread indicator has been enabled or disabled.
🧹 Nitpick comments (3)
src/tui/styles/mod.rs (1)
147-151: ⚖️ Poor tradeoffConsider avoiding double parse (optional refactor).
The TOML content is parsed twice: once in the caller (
toml::from_str::<Theme>) and again here (content.parse::<toml::Table>()). Given that theme files are small and loaded infrequently, the performance impact is negligible, but you could eliminate the redundant parse by having the caller pass in both theThemeand the parsedtoml::Table.That said, the current approach keeps the API simple and the inefficiency is harmless in practice. Feel free to leave as-is unless you're already refactoring this area.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tui/styles/mod.rs` around lines 147 - 151, The TOML content is being parsed twice: once in the caller with toml::from_str::<Theme> and again in fill_unread_from_accent with content.parse::<toml::Table>(). To eliminate this redundant parse, modify the fill_unread_from_accent function signature to accept an additional parameter containing the already-parsed toml::Table instead of re-parsing the content string. Update the caller to parse the content once, extract the Theme, and pass both the Theme and the parsed toml::Table to fill_unread_from_accent. Then replace the parse and unwrap_or logic with a direct check on the passed-in table. Note: this is an optional refactor since the performance impact is negligible for small theme files loaded infrequently.src/tui/components/help.rs (1)
53-59: 💤 Low valueOptional micro-optimization and test coverage.
The feature gating logic is correct and the comment clearly explains the "why." Two optional improvements:
Hoist the feature check: Since
unread_enabled()returns the same value for all iterations, you could call it once before the loop at line 51 (let unread_enabled = crate::session::unread_enabled();) and reference the local variable inside the loop. This avoids N function calls, though the impact is negligible since help rendering isn't a hot path.Test coverage: Consider adding a test that verifies the
ToggleUnreadaction doesn't appear in the shortcuts list whenunread_enabled()is false. The existing tests in this file check for specific shortcuts being present (e.g.,help_lists_snooze,help_lists_command_palette), so a similar test for the absence of the unread toggle when disabled would lock in this gating behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tui/components/help.rs` around lines 53 - 59, Extract the `crate::session::unread_enabled()` function call outside the loop (before line 51) into a local variable to avoid redundant calls across iterations, then reference this variable in the conditional check inside the loop for the ToggleUnread action. Additionally, add a test case (similar to existing tests like `help_lists_snooze` and `help_lists_command_palette` in this file) that verifies the `ToggleUnread` action is properly excluded from the shortcuts list when `unread_enabled()` returns false, ensuring this gating behavior is locked in with test coverage.src/tui/dialogs/command_palette.rs (1)
97-101: 💤 Low valueOptional micro-optimization and test coverage.
The feature gating logic is correct and the comment clearly explains the reasoning. Two optional improvements:
Hoist the feature check: Since
unread_enabled()returns the same value for all iterations, you could call it once at the top ofbuiltin_commands(let unread_enabled = crate::session::unread_enabled();) and reference the local variable in the closure. This avoids multiple function calls, though the impact is negligible since palette creation isn't a hot path.Test coverage: Consider adding a test that verifies the
ToggleUnreadcommand doesn't appear in the built-in commands list whenunread_enabled()is false. The existingserve_command_only_with_featuretest (lines 645-650) demonstrates this pattern for the serve feature, so a similar test for unread gating would be straightforward and would lock in this behavior alongside the other drift guards.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tui/dialogs/command_palette.rs` around lines 97 - 101, To implement the optional micro-optimization, move the `crate::session::unread_enabled()` call outside of the closure in the `builtin_commands` function by capturing it as a local variable at the start of the function (let unread_enabled = crate::session::unread_enabled();), then reference this cached variable in the closure condition where you check if b.id == bindings::ActionId::ToggleUnread. Additionally, add a test case similar to the existing `serve_command_only_with_feature` test pattern (referenced at lines 645-650) to verify that the ToggleUnread command is filtered out from builtin_commands when unread_enabled() returns false, ensuring this feature gating behavior is locked in by the test suite.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tui/styles/mod.rs`:
- Around line 142-156: Add dedicated unit tests for the fill_unread_from_accent
function to ensure the unread-to-accent fallback behavior is properly covered.
Create two test cases: first, verify that when a TOML theme string omits the
unread field but defines an accent field, the resulting theme has unread equal
to accent; second, verify that when a TOML theme string explicitly includes an
unread field, that explicit value is preserved and not overridden by the accent
value. These tests will prevent future refactors from silently breaking this
important edge case logic.
---
Outside diff comments:
In `@src/tui/home/mod.rs`:
- Line 5421: The call to crate::session::set_unread_enabled() at line 5421
changes the process-wide unread flag but does not rebuild the flat_items list,
causing the Attention sort ordering to become stale until a later rebuild. After
calling set_unread_enabled(), check if the Attention sort is currently active
and if so, trigger a rebuild of the item rows to ensure they are correctly
ordered according to the new unread flag state. This ensures rows are
immediately promoted or demoted from Attention-sort promotion based on whether
the unread indicator has been enabled or disabled.
---
Nitpick comments:
In `@src/tui/components/help.rs`:
- Around line 53-59: Extract the `crate::session::unread_enabled()` function
call outside the loop (before line 51) into a local variable to avoid redundant
calls across iterations, then reference this variable in the conditional check
inside the loop for the ToggleUnread action. Additionally, add a test case
(similar to existing tests like `help_lists_snooze` and
`help_lists_command_palette` in this file) that verifies the `ToggleUnread`
action is properly excluded from the shortcuts list when `unread_enabled()`
returns false, ensuring this gating behavior is locked in with test coverage.
In `@src/tui/dialogs/command_palette.rs`:
- Around line 97-101: To implement the optional micro-optimization, move the
`crate::session::unread_enabled()` call outside of the closure in the
`builtin_commands` function by capturing it as a local variable at the start of
the function (let unread_enabled = crate::session::unread_enabled();), then
reference this cached variable in the closure condition where you check if b.id
== bindings::ActionId::ToggleUnread. Additionally, add a test case similar to
the existing `serve_command_only_with_feature` test pattern (referenced at lines
645-650) to verify that the ToggleUnread command is filtered out from
builtin_commands when unread_enabled() returns false, ensuring this feature
gating behavior is locked in by the test suite.
In `@src/tui/styles/mod.rs`:
- Around line 147-151: The TOML content is being parsed twice: once in the
caller with toml::from_str::<Theme> and again in fill_unread_from_accent with
content.parse::<toml::Table>(). To eliminate this redundant parse, modify the
fill_unread_from_accent function signature to accept an additional parameter
containing the already-parsed toml::Table instead of re-parsing the content
string. Update the caller to parse the content once, extract the Theme, and pass
both the Theme and the parsed toml::Table to fill_unread_from_accent. Then
replace the parse and unwrap_or logic with a direct check on the passed-in
table. Note: this is an optional refactor since the performance impact is
negligible for small theme files loaded infrequently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f59c7bc6-1da4-45e5-bdd3-fa607f909bbb
📒 Files selected for processing (9)
src/session/groups.rssrc/tui/components/help.rssrc/tui/dialogs/command_palette.rssrc/tui/dialogs/context_menu.rssrc/tui/home/bindings.rssrc/tui/home/mod.rssrc/tui/styles/mod.rssrc/tui/styles/themes.rsthemes/builtin/catppuccin-latte.toml
🚧 Files skipped from review as they are similar to previous changes (5)
- themes/builtin/catppuccin-latte.toml
- src/tui/home/bindings.rs
- src/tui/styles/themes.rs
- src/tui/dialogs/context_menu.rs
- src/session/groups.rs
Member
|
I love the idea and I think it would be amazing to get it in the web ui as well, with a right click |
Eric162
added a commit
to Eric162/agent-of-empires
that referenced
this pull request
Jun 15, 2026
CodeRabbit follow-up on agent-of-empires#2088: add unit tests for `fill_unread_from_accent` so the fallback can't break silently. Covers a custom theme that omits `unread` (inherits its own accent) and one that sets it explicitly (kept, not overwritten). 🤖 Generated with Claude Code
Eric162
added a commit
to Eric162/agent-of-empires
that referenced
this pull request
Jun 16, 2026
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
Eric162
added a commit
to Eric162/agent-of-empires
that referenced
this pull request
Jun 16, 2026
CodeRabbit follow-up on agent-of-empires#2088: add unit tests for `fill_unread_from_accent` so the fallback can't break silently. Covers a custom theme that omits `unread` (inherits its own accent) and one that sets it explicitly (kept, not overwritten). 🤖 Generated with Claude Code
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tui/home/operations.rs`:
- Around line 1268-1269: After toggling the unread status with
`inst.toggle_unread()` and rebuilding `flat_items` with
`self.build_flat_items()`, the cursor position may shift to a different row when
items are reordered in Attention sort mode. Reseat the cursor by the item's `id`
after rebuilding flat_items to ensure the selection remains anchored to the same
session despite any rank reordering, preventing subsequent actions from
operating on the wrong item.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 25f48679-ebbf-4d4a-ba03-a4792db4ef28
📒 Files selected for processing (24)
src/session/config.rssrc/session/groups.rssrc/session/instance.rssrc/session/mod.rssrc/tui/app.rssrc/tui/components/help.rssrc/tui/dialogs/command_palette.rssrc/tui/dialogs/context_menu.rssrc/tui/home/bindings.rssrc/tui/home/input.rssrc/tui/home/mod.rssrc/tui/home/operations.rssrc/tui/home/render.rssrc/tui/home/tests.rssrc/tui/styles/mod.rssrc/tui/styles/themes.rsthemes/builtin/catppuccin-latte.tomlthemes/builtin/deep-ocean.tomlthemes/builtin/dracula.tomlthemes/builtin/empire.tomlthemes/builtin/phosphor.tomlthemes/builtin/rose-pine.tomlthemes/builtin/tokyo-night-storm.tomlthemes/builtin/zinc.toml
✅ Files skipped from review due to trivial changes (8)
- themes/builtin/zinc.toml
- themes/builtin/empire.toml
- themes/builtin/phosphor.toml
- themes/builtin/rose-pine.toml
- themes/builtin/tokyo-night-storm.toml
- src/tui/home/tests.rs
- themes/builtin/dracula.toml
- themes/builtin/deep-ocean.toml
🚧 Files skipped from review as they are similar to previous changes (15)
- themes/builtin/catppuccin-latte.toml
- src/tui/app.rs
- src/tui/dialogs/command_palette.rs
- src/tui/components/help.rs
- src/session/config.rs
- src/session/mod.rs
- src/tui/home/input.rs
- src/session/instance.rs
- src/tui/home/bindings.rs
- src/tui/home/render.rs
- src/tui/styles/mod.rs
- src/tui/home/mod.rs
- src/tui/styles/themes.rs
- src/tui/dialogs/context_menu.rs
- src/session/groups.rs
Member
|
Nevermind you pushed 13 hours ago I thought it was a few days |
Contributor
Author
|
@Seluj78 yes - let me just add the web ui stuff and get it ready for push. i'll tag you then. |
njbrake
reviewed
Jun 17, 2026
njbrake
left a comment
njbrake
left a comment
Collaborator
There was a problem hiding this comment.
Note: this review was drafted by Claude via back-and-forth with @njbrake. The reasoning and decisions are his; the prose is Claude's.
Review
Summary
Adds a quiet, persistent unread indicator for sessions in the TUI, complementing the existing loud, ephemeral turn-finish notifications. Two flavors: auto-unread (set on Running -> Idle, cleared by viewing/attaching) and manual-unread (v toggle, survives viewing). Surfaced as a theme.unread row color, a right-click menu entry, an Attention-sort promoter just below Waiting, and a stop in the w jump cycle. Gated behind a new default-on session.unread_indicator setting wired through the single-source schema.
I built the branch, ran cargo clippy, and ran the new unread tests locally; all clean.
Code review
session/instance.rs(UnreadKind, mutators, merge): clean.mark_unread_autonever clobbers Manual,mark_read_autoclears only Auto,toggle_unreadround-trips. Serde omitsNone. Well unit-tested.session/groups.rs(attention_rank): nicely done. Pure, testable, monotonic shift so a feature-on run with no unread rows orders identically to before. Folded into bothattention_session_keyandattention_group_key.session/mod.rs(atomic flag): process-wideAtomicBoolkeeps the hot sort path off a threaded flag. Reasonable; the cost is the apply-path sync bug noted below.styles/(theme.unread,fill_unread_from_accent): accent fallback for themes that omitunreadis correct and tested. The double parse is harmless for small theme files.- input / bindings / context_menu / help / palette: thorough feature gating via
Context::UnreadEnabled, with help and palette filtered separately and the menu row hidden when off.v/Vhas no conflicts. home/mod.rs(auto-marking, clear-on-view): live-send exemption and attach-return clear are correct;clear_auto_unreadruns regardless of the flag so a stale Auto cannot survive a disable then re-enable. Skips disk writes when already-unread/read to avoid flock churn.
Compliance with project guidelines
Good. The setting is added the single-source way (one #[setting(...)] annotation, surfaces in TUI and web automatically), no dead code, conventional commits, and doc comments explain the why. The e2e skip is justified (state is color-only; the harness asserts on screen text) and is compensated with solid unit coverage.
Automated review check-in (CodeRabbit)
Most findings were addressed in follow-up commits (group-key promotion, the v quick-pick, binding gating, unconditional clear_auto_unread, the accent fallback, the latte color, fallback tests). Two remained open on the latest run, and I agree with both:
-
Stale Attention sort on config flip (
apply_config_to_stateinhome/mod.rs). Real bug.switch_profilecallsreload(), which rebuildsflat_itemsunder the old flag, beforerefresh_from_config()flips it, and the interactive settings-close path never rebuilds afterward. So toggling the setting or switching to a profile with a different value leaves the Attention order stale until the next status tick. The row color self-corrects at render; only the ordering lags. Suggested fix: when the gate actually changes and Attention sort is active, rebuildflat_itemsand reseat the cursor by id. -
Cursor reseat after
toggle_unread_at_cursor(operations.rs). True but low priority. The cursor can shift after the reorder, but this matches the existingtoggle_favorite_at_cursorand unsnooze paths (neither reseats; onlytoggle_archivedoes). I would either leave it for consistency or fix all three together in a separate change rather than here.
The bigger question: readiness
From the thread, this is not only a code question. The shape (default-on with an opt-out) matches the direction discussed here. Seluj78 asked for web UI parity (right-click mark read/unread in the dashboard), and Eric162 replied that he would add the web UI piece and tag reviewers when ready. That web piece is not in the branch yet (the PR's own coverage analysis marks web as N/A). So by the author's own note this is still in progress.
Verdict
- Implementation (TUI scope): solid, well-tested, idiomatic, with one minor staleness bug to fix.
- Project fit: accept. Both maintainers want it, and the default-on plus opt-out shape matches the stated preference.
The two axes agree on direction, but I would hold the merge until the web dashboard support lands (or there is an explicit decision to split TUI now and web later), and fold in the staleness fix either way.
Eric162
added a commit
to Eric162/agent-of-empires
that referenced
this pull request
Jun 17, 2026
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
Eric162
added a commit
to Eric162/agent-of-empires
that referenced
this pull request
Jun 17, 2026
CodeRabbit follow-up on agent-of-empires#2088: add unit tests for `fill_unread_from_accent` so the fallback can't break silently. Covers a custom theme that omits `unread` (inherits its own accent) and one that sets it explicitly (kept, not overwritten). 🤖 Generated with Claude Code
Eric162
added a commit
to Eric162/agent-of-empires
that referenced
this pull request
Jun 17, 2026
CodeRabbit follow-up on agent-of-empires#2088: toggling unread changes a row's rank in the Attention sort, so rebuilding flat_items could move the cursor onto a different session and the next action would target the wrong one. Reseat by id after the rebuild, matching the snooze/archive paths. 🤖 Generated with Claude Code
Eric162
changed the title
Collaborator
|
Thanks! Yes I think I am comfortable merging this feature as default on and with a setting to let people disable the feature. I'm testing it out locally and I think the UI/UX needs some fixes. The highlighting is either too subtle or I'm doing something wrong? marking it read/unread doesn't seem to change anything when i'm not highlighting the session. Also, I know you probably picked 'v' as the hotkey because 'u' is already a hotkey for the update option, but I think the hotkey really should be 'u', and we just have some sort of handler to handle the case where that update footer is available so that when the footer is there 'u' routes to update but otherwise 'u' routes to read/unread? WDYT? Once you get the changes up and ready for review again, another short screen recording of how it looks in TUI as well as web would be great! 🙏 |
Member
|
On my end, I am waiting for you @Eric162 to ping me when the web part is ready to review and test ;) |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
web/src/components/__tests__/SessionRowTriage.test.tsx (1)
534-622: ⚡ Quick winAdd a multi-session unread regression test.
Current unread tests are single-session only. Please add a case where
sessions[1]is unread andsessions[0]is read, then verify “Mark as read” clears the aggregated unread state. This protects the workspace-level aggregation path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/__tests__/SessionRowTriage.test.tsx` around lines 534 - 622, Add a new test case within the "SessionRow unread" describe block that creates a workspace with multiple sessions where sessions[0] is read and sessions[1] is unread, render the Row for the unread session, trigger the "Mark as read" action via the context menu, and verify that the PATCH request is made correctly and that the workspace-level unread aggregation state is properly cleared after marking the session as read.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/api/sessions.rs`:
- Around line 2503-2508: The doc comment for the PATCH /api/sessions/{id}/unread
endpoint uses an em dash (`—`) separator, which violates the repository's Rust
comment style guidelines. Replace the em dash with a comma or semicolon to
maintain consistency with the codebase style rules. The em dash appears in the
line starting with the backtick-enclosed endpoint path and precedes the text
"flag a session unread".
In `@src/server/mod.rs`:
- Around line 2897-2934: The auto-unread marking logic in this block only
applies to the tmux-polled instances path and misses ACP-routed status changes
that transition through apply_status_intent, causing web-only ACP users to never
accrue the unread indicator when turns finish. Locate where apply_status_intent
is called in the acp_event_listener and add equivalent auto-unread handling
after that call: capture the status before calling apply_status_intent, then
check if the transition is Running to Idle with unread not set, and if so call
mark_unread_auto on the instance and persist the update using the same
persist_session_update pattern as this block, collecting the profile and
instance id for the persistence callback.
- Line 4690: The comment near line 4690 uses `---` as separators which violates
the repo's coding guidelines against using hyphens or emdashes as separators in
comments. Remove the `---` hyphens from both the beginning and end of the
comment while preserving the issue reference `#2248` and the description text
about the structured session healing behavior.
- Around line 2919-2932: The code currently drops storage errors from the
api::persist_session_update call by using let _ = which makes failures
undiagnosable. Instead of discarding the result, capture the Result returned by
the await on api::persist_session_update and log a warning message if an error
occurs. This will make it visible when the auto-unread persistence fails,
allowing the issue to be diagnosed and potentially retried on the next Running
-> Idle transition.
- Around line 652-655: The unread feature gate is only set once at daemon
startup in the initialization block but is never refreshed when users change the
setting through the dashboard. To fix this, locate the `update_settings` and
`update_profile_settings` handler functions in `src/server/api/system.rs`, and
after each one persists the updated configuration, add a call to
`crate::session::set_unread_enabled(...)` passing the new
`config.session.unread_indicator` value. This ensures the
`update_session_unread` function will use the current setting instead of the
stale startup value, allowing manual unread toggles to work correctly
immediately after the user changes the setting.
In `@web/src/App.tsx`:
- Around line 116-117: The unreadIndicatorEnabled state is initialized once and
never updates when session.unread_indicator changes at runtime, causing the
global context provider to serve stale values. Replace the static useState
initialization and one-time effect (the code around line 145-149 where
unreadIndicatorEnabled is set) with a mechanism that derives
unreadIndicatorEnabled from the live settings source and updates whenever
settings are saved or refetched. Ensure the effect that updates this state has
appropriate dependencies on the settings object so it re-syncs whenever the
session.unread_indicator property changes, and verify the context provider at
lines 188-191 exposes the current dynamic value rather than a frozen one.
In `@web/src/hooks/useSidebarTriage.ts`:
- Around line 95-107: The unread function in useSidebarTriage always operates on
the first session (ws.sessions[0]) via the sessionId variable, but the
workspace's unread state aggregates across all sessions. When marking as read
(markUnread is false), if a different session is unread, the setSessionUnread
call may no-op server-side while the optimistic update still sets unread to
null, causing desync. Instead of always targeting ws.sessions[0], determine
which session(s) should be marked read based on the marking operation - consider
whether you need to mark all sessions as read when marking the workspace as
read, or identify and target the specific unread session(s) to ensure the
server-side operation succeeds.
In `@web/src/lib/api.ts`:
- Around line 1397-1403: In the setSessionUnread function, the session id
parameter is being interpolated directly into the URL string without proper
encoding. This causes routing issues if the id contains reserved characters like
forward slashes. Wrap the id parameter with encodeURIComponent() when
constructing the fetch URL path to ensure special characters are properly
encoded before being sent to the API endpoint.
---
Nitpick comments:
In `@web/src/components/__tests__/SessionRowTriage.test.tsx`:
- Around line 534-622: Add a new test case within the "SessionRow unread"
describe block that creates a workspace with multiple sessions where sessions[0]
is read and sessions[1] is unread, render the Row for the unread session,
trigger the "Mark as read" action via the context menu, and verify that the
PATCH request is made correctly and that the workspace-level unread aggregation
state is properly cleared after marking the session as read.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c5bd5e5-c789-40aa-bab4-928749044ec6
📒 Files selected for processing (40)
src/server/api/mod.rssrc/server/api/sessions.rssrc/server/mod.rssrc/session/config.rssrc/session/groups.rssrc/session/instance.rssrc/session/mod.rssrc/tui/app.rssrc/tui/components/help.rssrc/tui/dialogs/command_palette.rssrc/tui/dialogs/context_menu.rssrc/tui/home/bindings.rssrc/tui/home/input.rssrc/tui/home/mod.rssrc/tui/home/operations.rssrc/tui/home/render.rssrc/tui/home/tests.rssrc/tui/styles/mod.rssrc/tui/styles/resolved.rssrc/tui/styles/themes.rsthemes/builtin/catppuccin-latte.tomlthemes/builtin/deep-ocean.tomlthemes/builtin/dracula.tomlthemes/builtin/empire.tomlthemes/builtin/phosphor.tomlthemes/builtin/rose-pine.tomlthemes/builtin/tokyo-night-storm.tomlthemes/builtin/zinc.tomlweb/src/App.tsxweb/src/components/WorkspaceSidebar.tsxweb/src/components/__tests__/SessionRowTriage.test.tsxweb/src/hooks/useSidebarTriage.tsweb/src/index.cssweb/src/lib/__tests__/sidebarOptimistic.test.tsweb/src/lib/api.test.tsweb/src/lib/api.tsweb/src/lib/sidebarOptimistic.tsweb/src/lib/types.tsweb/src/lib/unreadIndicator.tsweb/tests/coverage-matrix.json
✅ Files skipped from review due to trivial changes (8)
- web/src/index.css
- web/tests/coverage-matrix.json
- themes/builtin/empire.toml
- themes/builtin/zinc.toml
- themes/builtin/catppuccin-latte.toml
- themes/builtin/deep-ocean.toml
- themes/builtin/dracula.toml
- themes/builtin/phosphor.toml
🚧 Files skipped from review as they are similar to previous changes (17)
- themes/builtin/rose-pine.toml
- themes/builtin/tokyo-night-storm.toml
- src/tui/dialogs/command_palette.rs
- src/session/config.rs
- src/tui/home/tests.rs
- src/tui/home/render.rs
- src/tui/styles/themes.rs
- src/tui/components/help.rs
- src/tui/app.rs
- src/tui/styles/mod.rs
- src/tui/home/operations.rs
- src/session/groups.rs
- src/tui/home/bindings.rs
- src/session/instance.rs
- src/tui/home/mod.rs
- src/tui/dialogs/context_menu.rs
- src/session/mod.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 8
🧹 Nitpick comments (1)
web/src/components/__tests__/SessionRowTriage.test.tsx (1)
534-622: ⚡ Quick winAdd a multi-session unread regression test.
Current unread tests are single-session only. Please add a case where
sessions[1]is unread andsessions[0]is read, then verify “Mark as read” clears the aggregated unread state. This protects the workspace-level aggregation path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/__tests__/SessionRowTriage.test.tsx` around lines 534 - 622, Add a new test case within the "SessionRow unread" describe block that creates a workspace with multiple sessions where sessions[0] is read and sessions[1] is unread, render the Row for the unread session, trigger the "Mark as read" action via the context menu, and verify that the PATCH request is made correctly and that the workspace-level unread aggregation state is properly cleared after marking the session as read.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/api/sessions.rs`:
- Around line 2503-2508: The doc comment for the PATCH /api/sessions/{id}/unread
endpoint uses an em dash (`—`) separator, which violates the repository's Rust
comment style guidelines. Replace the em dash with a comma or semicolon to
maintain consistency with the codebase style rules. The em dash appears in the
line starting with the backtick-enclosed endpoint path and precedes the text
"flag a session unread".
In `@src/server/mod.rs`:
- Around line 2897-2934: The auto-unread marking logic in this block only
applies to the tmux-polled instances path and misses ACP-routed status changes
that transition through apply_status_intent, causing web-only ACP users to never
accrue the unread indicator when turns finish. Locate where apply_status_intent
is called in the acp_event_listener and add equivalent auto-unread handling
after that call: capture the status before calling apply_status_intent, then
check if the transition is Running to Idle with unread not set, and if so call
mark_unread_auto on the instance and persist the update using the same
persist_session_update pattern as this block, collecting the profile and
instance id for the persistence callback.
- Line 4690: The comment near line 4690 uses `---` as separators which violates
the repo's coding guidelines against using hyphens or emdashes as separators in
comments. Remove the `---` hyphens from both the beginning and end of the
comment while preserving the issue reference `#2248` and the description text
about the structured session healing behavior.
- Around line 2919-2932: The code currently drops storage errors from the
api::persist_session_update call by using let _ = which makes failures
undiagnosable. Instead of discarding the result, capture the Result returned by
the await on api::persist_session_update and log a warning message if an error
occurs. This will make it visible when the auto-unread persistence fails,
allowing the issue to be diagnosed and potentially retried on the next Running
-> Idle transition.
- Around line 652-655: The unread feature gate is only set once at daemon
startup in the initialization block but is never refreshed when users change the
setting through the dashboard. To fix this, locate the `update_settings` and
`update_profile_settings` handler functions in `src/server/api/system.rs`, and
after each one persists the updated configuration, add a call to
`crate::session::set_unread_enabled(...)` passing the new
`config.session.unread_indicator` value. This ensures the
`update_session_unread` function will use the current setting instead of the
stale startup value, allowing manual unread toggles to work correctly
immediately after the user changes the setting.
In `@web/src/App.tsx`:
- Around line 116-117: The unreadIndicatorEnabled state is initialized once and
never updates when session.unread_indicator changes at runtime, causing the
global context provider to serve stale values. Replace the static useState
initialization and one-time effect (the code around line 145-149 where
unreadIndicatorEnabled is set) with a mechanism that derives
unreadIndicatorEnabled from the live settings source and updates whenever
settings are saved or refetched. Ensure the effect that updates this state has
appropriate dependencies on the settings object so it re-syncs whenever the
session.unread_indicator property changes, and verify the context provider at
lines 188-191 exposes the current dynamic value rather than a frozen one.
In `@web/src/hooks/useSidebarTriage.ts`:
- Around line 95-107: The unread function in useSidebarTriage always operates on
the first session (ws.sessions[0]) via the sessionId variable, but the
workspace's unread state aggregates across all sessions. When marking as read
(markUnread is false), if a different session is unread, the setSessionUnread
call may no-op server-side while the optimistic update still sets unread to
null, causing desync. Instead of always targeting ws.sessions[0], determine
which session(s) should be marked read based on the marking operation - consider
whether you need to mark all sessions as read when marking the workspace as
read, or identify and target the specific unread session(s) to ensure the
server-side operation succeeds.
In `@web/src/lib/api.ts`:
- Around line 1397-1403: In the setSessionUnread function, the session id
parameter is being interpolated directly into the URL string without proper
encoding. This causes routing issues if the id contains reserved characters like
forward slashes. Wrap the id parameter with encodeURIComponent() when
constructing the fetch URL path to ensure special characters are properly
encoded before being sent to the API endpoint.
---
Nitpick comments:
In `@web/src/components/__tests__/SessionRowTriage.test.tsx`:
- Around line 534-622: Add a new test case within the "SessionRow unread"
describe block that creates a workspace with multiple sessions where sessions[0]
is read and sessions[1] is unread, render the Row for the unread session,
trigger the "Mark as read" action via the context menu, and verify that the
PATCH request is made correctly and that the workspace-level unread aggregation
state is properly cleared after marking the session as read.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c5bd5e5-c789-40aa-bab4-928749044ec6
📒 Files selected for processing (40)
src/server/api/mod.rssrc/server/api/sessions.rssrc/server/mod.rssrc/session/config.rssrc/session/groups.rssrc/session/instance.rssrc/session/mod.rssrc/tui/app.rssrc/tui/components/help.rssrc/tui/dialogs/command_palette.rssrc/tui/dialogs/context_menu.rssrc/tui/home/bindings.rssrc/tui/home/input.rssrc/tui/home/mod.rssrc/tui/home/operations.rssrc/tui/home/render.rssrc/tui/home/tests.rssrc/tui/styles/mod.rssrc/tui/styles/resolved.rssrc/tui/styles/themes.rsthemes/builtin/catppuccin-latte.tomlthemes/builtin/deep-ocean.tomlthemes/builtin/dracula.tomlthemes/builtin/empire.tomlthemes/builtin/phosphor.tomlthemes/builtin/rose-pine.tomlthemes/builtin/tokyo-night-storm.tomlthemes/builtin/zinc.tomlweb/src/App.tsxweb/src/components/WorkspaceSidebar.tsxweb/src/components/__tests__/SessionRowTriage.test.tsxweb/src/hooks/useSidebarTriage.tsweb/src/index.cssweb/src/lib/__tests__/sidebarOptimistic.test.tsweb/src/lib/api.test.tsweb/src/lib/api.tsweb/src/lib/sidebarOptimistic.tsweb/src/lib/types.tsweb/src/lib/unreadIndicator.tsweb/tests/coverage-matrix.json
✅ Files skipped from review due to trivial changes (8)
- web/src/index.css
- web/tests/coverage-matrix.json
- themes/builtin/empire.toml
- themes/builtin/zinc.toml
- themes/builtin/catppuccin-latte.toml
- themes/builtin/deep-ocean.toml
- themes/builtin/dracula.toml
- themes/builtin/phosphor.toml
🚧 Files skipped from review as they are similar to previous changes (17)
- themes/builtin/rose-pine.toml
- themes/builtin/tokyo-night-storm.toml
- src/tui/dialogs/command_palette.rs
- src/session/config.rs
- src/tui/home/tests.rs
- src/tui/home/render.rs
- src/tui/styles/themes.rs
- src/tui/components/help.rs
- src/tui/app.rs
- src/tui/styles/mod.rs
- src/tui/home/operations.rs
- src/session/groups.rs
- src/tui/home/bindings.rs
- src/session/instance.rs
- src/tui/home/mod.rs
- src/tui/dialogs/context_menu.rs
- src/session/mod.rs
🛑 Comments failed to post (8)
src/server/api/sessions.rs (1)
2503-2508:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPlease replace the em dash in the endpoint doc comment.
Line 2503 uses an em dash separator (
—), which violates the repo’s Rust comment style rule. Please rephrase with a comma or semicolon.Suggested edit
-/// `PATCH /api/sessions/{id}/unread` — flag a session unread (`{"unread":true}`) +/// `PATCH /api/sessions/{id}/unread`, flag a session unread (`{"unread":true}`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./// `PATCH /api/sessions/{id}/unread`, flag a session unread (`{"unread":true}`) /// or mark it read (`{"unread":false}`). Mirrors the TUI's `v` toggle, but the /// client computes the target from the current state rather than toggling /// server-side, so an optimistic UI update can't desync. No-op when the /// `session.unread_indicator` feature is off (the client hides the control /// then, but guard here too). Persist-then-mutate, like snooze.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/api/sessions.rs` around lines 2503 - 2508, The doc comment for the PATCH /api/sessions/{id}/unread endpoint uses an em dash (`—`) separator, which violates the repository's Rust comment style guidelines. Replace the em dash with a comma or semicolon to maintain consistency with the codebase style rules. The em dash appears in the line starting with the backtick-enclosed endpoint path and precedes the text "flag a session unread".Source: Coding guidelines
src/server/mod.rs (4)
652-655:
⚠️ Potential issue | 🟡 Minor🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Verify the unread feature gate is refreshed after settings changes, not only during serve startup. # Expectation: at least one settings-update path calls set_unread_enabled(...) after accepting session.unread_indicator. rg -n -C4 '\bset_unread_enabled\s*\(' --glob '*.rs' rg -n -C6 '\bunread_indicator\b|\bupdate_settings\b|\bvalidate_patch\b' --glob '*.rs'Repository: agent-of-empires/agent-of-empires
Length of output: 44839
Refresh the unread feature gate when the setting changes, not only at startup.
The server currently loads
session.unread_indicatoronce during daemon startup (line 655) but does not refresh it when a user changes the setting via the dashboard. This means toggling the unread indicator in settings won't take effect in the API until the daemon restarts.The unread gate is checked in
update_session_unread(line 2547 insrc/server/api/sessions.rs), so if it's stale, the PATCH endpoint will silently ignore the user's manual unread toggles.Fix: Call
crate::session::set_unread_enabled(...)in bothupdate_settingsandupdate_profile_settingshandlers (insrc/server/api/system.rs) after the config is persisted, the same way the TUI does on config refresh (lines 1245 and 5561 insrc/tui/home/mod.rs).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/mod.rs` around lines 652 - 655, The unread feature gate is only set once at daemon startup in the initialization block but is never refreshed when users change the setting through the dashboard. To fix this, locate the `update_settings` and `update_profile_settings` handler functions in `src/server/api/system.rs`, and after each one persists the updated configuration, add a call to `crate::session::set_unread_enabled(...)` passing the new `config.session.unread_indicator` value. This ensures the `update_session_unread` function will use the current setting instead of the stale startup value, allowing manual unread toggles to work correctly immediately after the user changes the setting.
2897-2934:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftShare auto-unread marking with the ACP status path.
This block only sees the tmux-polled rows before
reload_state_instances_from_disk()reapplies the ACP overlay. Structured sessions transition throughacp_event_listener→apply_status_intent, whereEvent::StoppedbecomesStatus::Idle; after that, the next poll seesprev == Idle, so a web-only ACP turn can finish without ever being auto-marked unread. Please mark and persist unread on the sharedRunning -> Idletransition path, or add the equivalent handling in the ACP listener afterapply_status_intent.A compact shape for the ACP-side guard would be:
let before = inst.status; apply_status_intent(inst, status_intent, &state.status_tx); let auto_unread_profile = if crate::session::unread_enabled() && before == Status::Running && inst.status == Status::Idle && inst.unread.is_none() { inst.mark_unread_auto(); Some(inst.source_profile.clone()) } else { None };Based on PR objectives, auto-unread should be set when a turn finishes so web-only users accrue the indicator.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/mod.rs` around lines 2897 - 2934, The auto-unread marking logic in this block only applies to the tmux-polled instances path and misses ACP-routed status changes that transition through apply_status_intent, causing web-only ACP users to never accrue the unread indicator when turns finish. Locate where apply_status_intent is called in the acp_event_listener and add equivalent auto-unread handling after that call: capture the status before calling apply_status_intent, then check if the transition is Running to Idle with unread not set, and if so call mark_unread_auto on the instance and persist the update using the same persist_session_update pattern as this block, collecting the profile and instance id for the persistence callback.
2919-2932:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLog auto-unread persistence failures.
let _ =drops storage errors here. If the write fails, the in-memory marker may disappear on the next disk reload, and theRunning -> Idleedge will not happen again to retry. A warning is enough to make this diagnosable.Suggested error handling
for (profile, ids) in newly_idle { - let _ = api::persist_session_update( + let profile_for_log = profile.clone(); + if let Err(e) = api::persist_session_update( profile, "auto-unread", state.file_watch.clone(), move |insts| { for inst in insts.iter_mut() { @@ } }, ) - .await; + .await + { + tracing::warn!( + target: "server.unread", + profile = %profile_for_log, + error = %e, + "failed to persist auto-unread marker" + ); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/mod.rs` around lines 2919 - 2932, The code currently drops storage errors from the api::persist_session_update call by using let _ = which makes failures undiagnosable. Instead of discarding the result, capture the Result returned by the await on api::persist_session_update and log a warning message if an error occurs. This will make it visible when the auto-unread persistence fails, allowing the issue to be diagnosed and potentially retried on the next Running -> Idle transition.
4690-4690:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the separator hyphens from the test comment.
Line 4690 uses
---as a prose separator, which this repo avoids in human-authored comments.Suggested cleanup
- // --- `#2248`: a structured session must heal out of a stale Stopped --- + // `#2248`: a structured session must heal out of a stale Stopped.As per coding guidelines, “Do not use emdashes or
--as separators in docs and comments.”📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// `#2248`: a structured session must heal out of a stale Stopped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/mod.rs` at line 4690, The comment near line 4690 uses `---` as separators which violates the repo's coding guidelines against using hyphens or emdashes as separators in comments. Remove the `---` hyphens from both the beginning and end of the comment while preserving the issue reference `#2248` and the description text about the structured session healing behavior.Source: Coding guidelines
web/src/App.tsx (1)
116-117:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnread-indicator setting is effectively “load-time only” in the app shell.
Line 145-149 sets
unreadIndicatorEnabledonce, and Line 188-191 provides that frozen value globally. Togglingsession.unread_indicatorduring runtime won’t update unread behavior until reload, so clear-on-view and row markers can stay out of sync with current settings.Please source this context value from a live settings signal (or re-sync this state whenever settings are saved/refetched), not only the initial fetch.
Also applies to: 144-149, 188-191
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/App.tsx` around lines 116 - 117, The unreadIndicatorEnabled state is initialized once and never updates when session.unread_indicator changes at runtime, causing the global context provider to serve stale values. Replace the static useState initialization and one-time effect (the code around line 145-149 where unreadIndicatorEnabled is set) with a mechanism that derives unreadIndicatorEnabled from the live settings source and updates whenever settings are saved or refetched. Ensure the effect that updates this state has appropriate dependencies on the settings object so it re-syncs whenever the session.unread_indicator property changes, and verify the context provider at lines 188-191 exposes the current dynamic value rather than a frozen one.web/src/hooks/useSidebarTriage.ts (1)
95-107:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnread “mark read” can desync on multi-session workspaces.
Line 97 always targets
ws.sessions[0], but the row’s unread state is aggregated from any session. If a different session is unread, “Mark as read” can no-op server-side while optimisticunread: nullremains, leaving the row falsely read.💡 Proposed fix
const unread = useCallback( async (ws: Workspace, markUnread: boolean): Promise<TriageResult> => { - const sessionId = ws.sessions[0]?.id; - if (!sessionId) return { workspaceId: ws.id, ok: false, skipped: true }; + const targetSessionIds = markUnread + ? (ws.sessions[0]?.id ? [ws.sessions[0].id] : []) + : ws.sessions.filter((s) => s.unread != null).map((s) => s.id); + if (targetSessionIds.length === 0) { + return { workspaceId: ws.id, ok: false, skipped: true }; + } // "Mark as unread" sets a manual flag; "Mark as read" clears it. setOverride(ws.id, { unread: markUnread ? "manual" : null }); - const result = await setSessionUnread(sessionId, markUnread); - if (!result) { - setOverride(ws.id, { unread: undefined }); - return { workspaceId: ws.id, ok: false }; + for (const sessionId of targetSessionIds) { + const result = await setSessionUnread(sessionId, markUnread); + if (!result) { + setOverride(ws.id, { unread: undefined }); + return { workspaceId: ws.id, ok: false }; + } } return { workspaceId: ws.id, ok: true }; }, [setOverride], );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/hooks/useSidebarTriage.ts` around lines 95 - 107, The unread function in useSidebarTriage always operates on the first session (ws.sessions[0]) via the sessionId variable, but the workspace's unread state aggregates across all sessions. When marking as read (markUnread is false), if a different session is unread, the setSessionUnread call may no-op server-side while the optimistic update still sets unread to null, causing desync. Instead of always targeting ws.sessions[0], determine which session(s) should be marked read based on the marking operation - consider whether you need to mark all sessions as read when marking the workspace as read, or identify and target the specific unread session(s) to ensure the server-side operation succeeds.web/src/lib/api.ts (1)
1397-1403:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winEncode session IDs in the unread PATCH route.
At Line 1399,
idis interpolated without URL encoding. If a session ID contains/or other reserved characters, this request can route incorrectly and fail to clear/toggle unread.Suggested fix
export async function setSessionUnread(id: string, unread: boolean): Promise<SessionResponse | null> { try { - const res = await fetch(`/api/sessions/${id}/unread`, { + const res = await fetch(`/api/sessions/${encodeURIComponent(id)}/unread`, { method: "PATCH", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ unread }), });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/lib/api.ts` around lines 1397 - 1403, In the setSessionUnread function, the session id parameter is being interpolated directly into the URL string without proper encoding. This causes routing issues if the id contains reserved characters like forward slashes. Wrap the id parameter with encodeURIComponent() when constructing the fetch URL path to ensure special characters are properly encoded before being sent to the API endpoint.
Eric162
added a commit
to Eric162/agent-of-empires
that referenced
this pull request
Jun 17, 2026
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
Eric162
added a commit
to Eric162/agent-of-empires
that referenced
this pull request
Jun 18, 2026
Address @njbrake's review on agent-of-empires#2088: - TUI unread rows now read at a glance: a solid `●` icon + bold on top of the `theme.unread` color, instead of a too-subtle color-only swap. Mirrors the web sidebar's unread dot. - Move the unread toggle from `v` to `u`, the more intuitive key. `u` routes by context: it triggers Update while an update is available (the footer is showing), otherwise it toggles read/unread. Implemented via a new `Context::UpdateAvailable` gate on the Update binding, with ToggleUnread listed after it as the fallback (both modes). The context-menu quick-pick and docs follow. 🤖 Generated with Claude Code
Eric162
added a commit
to Eric162/agent-of-empires
that referenced
this pull request
Jun 18, 2026
Two fixes from local testing of agent-of-empires#2088: - The selected/open session no longer hides a *manual* unread flag. The active-row suppression now applies only to `auto` markers (which clear-on-view removes a beat later, so hiding them avoids a flash); a deliberate "mark unread" stays visible while the session is selected. - The unread row's *text* now takes the unread color (with bold), not just a bolded primary color, so the whole row recolors the way a Running row does and matches the TUI. 🤖 Generated with Claude Code
Eric162
added a commit
to Eric162/agent-of-empires
that referenced
this pull request
Jun 18, 2026
agent-of-empires#2088 review: a Running session that also carried an unread marker kept showing the solid unread dot — the color was gated to resting states (Idle/Unknown) but the icon + bold weren't, so the dot replaced the running spinner. Gate the whole unread treatment (color, dot, bold) on resting status so any live status (Running/Waiting/Starting/...) keeps its own color and spinner. Adds a render regression test. 🤖 Generated with Claude Code
Eric162
added a commit
to Eric162/agent-of-empires
that referenced
this pull request
Jun 18, 2026
Adversarial-review findings on agent-of-empires#2088: - Sunk group members no longer promote a group as unread. `attention_group_key` aggregated `is_unread()` across all members, including archived/snoozed (tier-99) ones — and archive/snooze don't clear `unread` — so a dismissed session could drag its whole group to unread rank 1. Now only non-sunk members count, matching the session key's tier-99 short-circuit. Regression test added. - Mark `session.unread_indicator` global_only. It's backed by one process-wide flag refreshed from the active profile, so a per-profile override couldn't be honored; exposing it as profile-overridable was misleading. Scope it global to match the implementation. 🤖 Generated with Claude Code
Collaborator
Sounds good to me! Out of curiousity I looked and the command to mark read/unread for the macos mail app is cmd+U (uppercase U) so we are at least matching the convention of one popular email client. |
Adds a non-intrusive "unread" indicator on sessions, distinct from the existing turn-finish notifications (sound/popup). Two kinds: - auto-unread: set when a turn finishes (Running -> Idle), cleared by viewing the session (Tab into live-send, Enter to attach) or the manual toggle. The actively-viewed session is exempt, so finishing a turn while you're watching it doesn't mark it unread. - manual-unread: a deliberate "flag for later" toggled with `v`, cleared only by the manual toggle (survives Tab/Enter). Surfaced as a dedicated `theme.unread` row color on resting (Idle/Unknown) rows, a context-menu entry, and an Attention-sort promoter that floats unread rows just below Waiting. `w` (jump to next waiting/idle) also stops on unread rows. The whole feature is gated behind the `AOE_UNREAD` env var so it ships dark; when unset, behavior is identical to before. 🤖 Generated with Claude Code
Switch the unread feature from the `AOE_UNREAD` env testing-gate to a real config toggle, on by default: - Add `session.unread_indicator: bool` (default true) via the single-source settings schema, so it surfaces in the TUI and web settings automatically. - Replace the env-var `unread_enabled()` OnceLock with a config-backed AtomicBool (default true) refreshed by the TUI on startup and whenever config is re-applied, so a settings change takes effect without a restart. Keeps the cheap accessor the hot Attention-sort path relies on. - Update doc comments and context-menu tests for the now-default-on feature. 🤖 Generated with Claude Code
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
CodeRabbit follow-up on agent-of-empires#2088: add unit tests for `fill_unread_from_accent` so the fallback can't break silently. Covers a custom theme that omits `unread` (inherits its own accent) and one that sets it explicitly (kept, not overwritten). 🤖 Generated with Claude Code
CodeRabbit follow-up on agent-of-empires#2088: toggling unread changes a row's rank in the Attention sort, so rebuilding flat_items could move the cursor onto a different session and the next action would target the wrong one. Reseat by id after the rebuild, matching the snooze/archive paths. 🤖 Generated with Claude Code
Bring the unread feature to the web dashboard, matching the TUI:
Backend
- Expose `unread` ("auto"/"manual"/null) on SessionResponse.
- PATCH /api/sessions/{id}/unread ({unread:bool}): flag manually unread or
mark read (clears both kinds), mirroring the TUI toggle; persist-then-
mutate like snooze, read-only guarded, no-op when the feature is off.
- Auto-mark Running -> Idle in the server status poll loop (persisted per
profile) so a web-only user accrues the indicator with no TUI running.
- Feed the `unread_enabled()` gate from the daemon's resolved
`session.unread_indicator` config at startup.
- Project `theme.unread` to the `--color-status-unread` CSS var.
Frontend
- Unread dot on the sidebar row (theme-accent), suppressed on the active
row; a right-click "Mark as read/unread" menu item, both gated on the
`session.unread_indicator` setting via a new context.
- Optimistic overlay support for unread, like pin/archive/snooze.
- Clear-on-view: opening a session (or having it open when its turn
finishes) clears an `auto` marker; a `manual` flag survives viewing.
Tests: instance + serialization Rust tests; api/overlay/RTL vitest;
coverage-matrix entry (sidebar.unread-indicator).
🤖 Generated with Claude Code
Address @njbrake's review on agent-of-empires#2088: - TUI unread rows now read at a glance: a solid `●` icon + bold on top of the `theme.unread` color, instead of a too-subtle color-only swap. Mirrors the web sidebar's unread dot. - Move the unread toggle from `v` to `u`, the more intuitive key. `u` routes by context: it triggers Update while an update is available (the footer is showing), otherwise it toggles read/unread. Implemented via a new `Context::UpdateAvailable` gate on the Update binding, with ToggleUnread listed after it as the fallback (both modes). The context-menu quick-pick and docs follow. 🤖 Generated with Claude Code
…glyph Follow-ups from review: - Hotkey: instead of routing `u` contextually (Update-when-available, else unread), give each its own key — `u` toggles read/unread, `U` updates. They form the standard primary/secondary pair on the `u` letter, so strict mode relocates cleanly (unread -> `U`, update -> `Ctrl+U`). Drops the `Context::UpdateAvailable`/`Ctx.update_available` machinery. The update footer hint now reads its key from the binding registry so it can't drift. - Web: the unread marker now *replaces* the resting status glyph with a solid dot (in the unread color) rather than sitting beside the title, matching the TUI. Only for resting (Idle/Unknown) rows; a live spinner stays. 🤖 Generated with Claude Code
Two fixes from local testing of agent-of-empires#2088: - The selected/open session no longer hides a *manual* unread flag. The active-row suppression now applies only to `auto` markers (which clear-on-view removes a beat later, so hiding them avoids a flash); a deliberate "mark unread" stays visible while the session is selected. - The unread row's *text* now takes the unread color (with bold), not just a bolded primary color, so the whole row recolors the way a Running row does and matches the TUI. 🤖 Generated with Claude Code
agent-of-empires#2088 review: a Running session that also carried an unread marker kept showing the solid unread dot — the color was gated to resting states (Idle/Unknown) but the icon + bold weren't, so the dot replaced the running spinner. Gate the whole unread treatment (color, dot, bold) on resting status so any live status (Running/Waiting/Starting/...) keeps its own color and spinner. Adds a render regression test. 🤖 Generated with Claude Code
Adversarial-review findings on agent-of-empires#2088: - Sunk group members no longer promote a group as unread. `attention_group_key` aggregated `is_unread()` across all members, including archived/snoozed (tier-99) ones — and archive/snooze don't clear `unread` — so a dismissed session could drag its whole group to unread rank 1. Now only non-sunk members count, matching the session key's tier-99 short-circuit. Regression test added. - Mark `session.unread_indicator` global_only. It's backed by one process-wide flag refreshed from the active profile, so a per-profile override couldn't be honored; exposing it as profile-overridable was misleading. Scope it global to match the implementation. 🤖 Generated with Claude Code
…read Replace the two-kind unread marker (Auto/Manual) with a single boolean `unread`. Engaging with a session (open/attach, live-send, click, web open) always clears it; `u` toggles it back on. Drops the kind enum, the kind-aware web optimistic reconciler, and the wire-format strings in favor of a bare boolean. Add dwell-to-read in the TUI: a session kept selected with the list in the foreground for UNREAD_DWELL (3s) clears its marker, distinguishing "scrolled past" from "stopped to read." Driven from the app tick loop, paused during dialogs/live-send, reset on selection change, and restarted on a manual toggle so re-flagging the current row isn't undone. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #2088 +/- ##
==========================================
- Coverage 84.26% 84.26% -0.01%
==========================================
Files 233 234 +1
Lines 13547 13600 +53
Branches 3954 3971 +17
==========================================
+ Hits 11416 11460 +44
- Misses 1822 1829 +7
- Partials 309 311 +2
... and 5 files with indirect coverage changes
Continue to review full report in Codecov by Harness.
|
njbrake
approved these changes
Jun 18, 2026
njbrake
left a comment
njbrake
left a comment
Collaborator
There was a problem hiding this comment.
Clean UX with easy to understand usage. Love it.
12 tasks
jerome-benoit
added a commit
to jerome-benoit/agent-of-empires
that referenced
this pull request
Jun 19, 2026
The unread row color #7dcfff (cyan) had a higher Rec. 601 luminance (188.0) than waiting #e0af68 (181.6), so an unread Idle row visually outshone a Waiting peer on the dark-themed sort, contradicting the Attention-sort promoter contract from agent-of-empires#2088 that floats unread rows just below Waiting. Cyan is also off the upstream spec slot for info: tokyonight.nvim defines colors.info = colors.blue2 = #0db9d7 directly. Switch to blue2 #0db9d7 (Rec. 601 = 137.0). The replacement sits between waiting (181.6) and idle (97.1) on dark bg, restores the ordering under both Rec. 601 and WCAG 2.1 relative luminance, matches the upstream info slot, and stays inside the Storm palette. Extend theme_attention_hierarchy_holds to also assert waiting > unread > idle for every builtin theme so future drift is caught at test time. The new assertion fails on the prior #7dcfff with the exact panic 'tokyo-night-storm (dark bg): waiting luminance 181.6 should exceed unread 188.0'. unread vs fresh_idle ordering is intentionally unconstrained so themes can tie them (empire and zinc do, at the same Tailwind step).
12 tasks
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds a non-intrusive unread indicator on sessions, in both the TUI and the web dashboard, built on top of the existing turn-finish detection (the
Running → Idletransition that already drives sound/popup notifications). Notifications are loud and ephemeral; this is the quiet, persistent complement: a glanceable cue distinguishing "this turn finished and I've looked" from "finished and I haven't."Two kinds of unread, distinguished only by what clears them:
Running → Idle). Cleared by viewing the session (TUI: Tab/Enter; web: opening it) or the manual toggle. The session you're actively viewing is exempt.Enabled by default, with a setting to turn it off
On by default, gated behind the
session.unread_indicatortoggle (Settings → Session → Interaction) through the single-source settings schema, so it shows up in both the TUI and web settings automatically. Turning it off disables the indicator, auto-marking, sort promotion, and the toggle on every surface.TUI
theme.unreadrow color on resting (Idle/Unknown) rows; auto-mark on turn finish with an active-view exemption;vto flag/unflag (gated binding, hidden from help/palette when off); a right-click context-menu entry; an Attention-sort promoter that floats unread rows just below Waiting;wjump-to-next includes unread.Web dashboard
--color-status-unread), suppressed on the active row; a right-click "Mark as read / unread" context-menu item — both gated on the setting.PATCH /api/sessions/{id}/unread({unread:bool});unreadexposed on the session API; optimistic overlay like pin/archive/snooze.Running → Idle(persisted), so a web-only user with no TUI running still accrues the indicator. Clear-on-view is client-driven (opening a session clears anautomarker; amanualflag survives).PR Type
Checklist
Test Coverage Analysis
Web dashboard / structured view
coverage-matrix.jsonentry (sidebar.unread-indicator): RTL coverage inSessionRowTriage.test.tsx(dot render, active-row suppression, menu label flip, PATCH payloads for mark read/unread, feature-off gating), request-shape + resolver tests inapi.test.tsandsidebarOptimistic.test.ts.TUI / CLI (e2e test)
Instancemutators + serde,attention_rankpromoter, thevbinding, the context-menu label flip,SessionResponseserialization, and the read-only-guard audit for the new endpoint.AI Usage
AI Model/Tool used: Claude Code (Opus 4.8)
Overview
This PR introduces a persistent “unread” session indicator across the TUI and web UI so users can immediately tell which idle sessions still need review versus sessions they’ve already seen.
What Changed
Unread tracking + controls
session.unread_indicator(enabled by default) in Settings → Session → Interaction, applied immediately (no restart).v(andVin strict mode) toggles unread for the selected session (only when enabled).Sorting + navigation behavior
wjump navigation now stops on unread sessions as well as waiting/idle sessions.Visual updates
theme.unread) and applied it to resting rows to visually distinguish unread sessions.Web UI support
Tests / QA
Benefits
Potential follow-ups