Feat/health and fixes#5
Merged
Merged
Conversation
Stream parser dropped already-parsed messages when hitting EOF mid-object at chunk boundaries (8KB reads vs ~32KB of pretty-printed JSON). Viewport rendered backward-only from anchor, causing blank screens on tall messages and showing only one message at scroll top. Scroll tracking used a heuristic that couldn't account for variable-height messages. - Stream: yield parsed values before returning Incomplete on partial tail - Viewport: rewrite compute_visible to anchor-based forward rendering - Scroll: track actual visible range from render pass, replace heuristic - Profile: remove relay status indicator rendering - Tests: 10 new tests covering all three fixes (248 total passing)
|
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:
WalkthroughAdds a Relay Health UI and data flow plus related state and effects: new health screen and rendering, new Action/Effect variants for loading relays and relay health, widget loading/visible-range changes, stream lifecycle/task aborts, Resize event payload removal, and improved JSON-stream parsing behavior. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app.rs (1)
1038-1047:⚠️ Potential issue | 🟠 MajorThe global
Tabhandler steals the Health screen toggle.This branch runs before
handle_health_key(), so with logs visible onScreen::Health,Tabswitches log tabs instead ofHealthView. The hint bar insrc/screen/health.rs:516-538becomes impossible to follow in that state.🔧 Minimal fix
- if key.code == KeyCode::Tab && self.show_logs && !matches!(self.screen, Screen::UserSearch) + if key.code == KeyCode::Tab + && self.show_logs + && !matches!(self.screen, Screen::UserSearch | Screen::Health) { self.log_tab = match self.log_tab { LogTab::Activity => LogTab::Daemon, LogTab::Daemon => LogTab::Activity, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` around lines 1038 - 1047, The Tab handling branch in the key handler steals the Health screen toggle; update the condition guarding the Tab branch (the block that sets self.log_tab, self.log_scroll and returns vec![]) so it does not run when self.screen is Screen::Health (in addition to excluding Screen::UserSearch). Locate the Tab branch in app.rs (the code that checks key.code == KeyCode::Tab && self.show_logs ...) and add a check like && !matches!(self.screen, Screen::Health) (or equivalent) so Tab will reach handle_health_key() when on the Health screen.
🧹 Nitpick comments (2)
src/screen/group_detail.rs (1)
174-186: Consider extracting a shared relay rendering helper.This
draw_relays(&[String], ...)is similar todraw_relays(&[Value], ...)inprofile.rs. While the input types differ (Stringfor group relays vsValuefor account relays), you could unify them with a trait or by converting to a common format before rendering.Not urgent — the current approach is straightforward and the duplication is minimal.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screen/group_detail.rs` around lines 174 - 186, The two similar functions draw_relays in group_detail.rs and profile.rs should be consolidated into a single helper that accepts a unified relay input (e.g., an iterator or slice of &str/impl AsRef<str> or a Vec<String>) and returns or renders the same Paragraph/lines; update group_detail::draw_relays and profile::draw_relays to call this new helper (or convert their input Value/String arrays to a Vec<&str> and pass it in). Locate the existing functions named draw_relays and extract the Span/Line construction and frame.render_widget call into a shared function (e.g., render_relays or build_relay_paragraph) so both modules reuse the same rendering logic while preserving styling (Style::default().fg(Color::DarkGray) and Color::White).src/widget/message_list.rs (1)
481-489: Consider usingOptionfor clearer semantics.
visible_rangereturns(0, 0)for both "no messages visible" and "only message 0 is visible". While the current implementation works (callers can checkmessages.is_empty()), anOption<(usize, usize)>return type would be more explicit.This is a minor observation for future consideration — the current behavior is documented and tested.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/widget/message_list.rs` around lines 481 - 489, Change visible_range to return Option<(usize, usize)> instead of (usize, usize): call self.compute_visible(area) as before, and if visible.first()/visible.last() yield indices return Some((first, last)), otherwise return None. Update the function signature visible_range(&self, area: Rect) -> Option<(usize, usize)> and adjust any callers that expect (0,0) to handle None (or map None to (0,0) where backward compatibility is required). Use the same pattern of matching on visible.first() and visible.last() as in the original implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app.rs`:
- Around line 2247-2250: The handler for KeyCode::Char('r') currently clears
self.relay_health before dispatching Effect::LoadRelayHealth, which drops the
last-good snapshot on failed reloads; instead, do not set self.relay_health =
None there—either leave the existing snapshot in place or add a separate loading
flag (e.g., health_loading) and set that to true while preserving
self.relay_health and self.health_scroll; only update or replace relay_health
when the RelayHealthLoaded event arrives (or clear it there on explicit
success/failure), ensuring UI fallbacks in src/screen/health.rs continue to show
the last-known state until the load completes.
In `@src/screen/health.rs`:
- Around line 133-140: The current collect closure deduplicates relays by
keeping the first (url, status) seen; change it to keep the worst status across
sessions by, for each relay URL, checking if out already contains that URL and
if so replacing its status with the worst of the existing and new status (e.g.,
map statuses to severity or implement a helper worst_status(existing: &str,
candidate: &str) -> String that returns "Disconnected" over "Connected" etc.),
otherwise push the (url, status) tuple; update the closure named collect and any
helper used so account_inbox/ephemeral rendering becomes order-independent.
- Line 291: Replace the manual ceiling division calculating num_rows with the
div_ceil helper: change the expression to use relays.len().div_ceil(&num_cols)
(or relays.len().div_ceil(num_cols) if using the inherent method on usize), and
add the appropriate import (e.g., use num_integer::Integer; if using
num_integer's trait) so Clippy no longer flags manual ceiling division; update
the expression that defines num_rows (the variable named num_rows that uses
relays and num_cols) accordingly.
---
Outside diff comments:
In `@src/app.rs`:
- Around line 1038-1047: The Tab handling branch in the key handler steals the
Health screen toggle; update the condition guarding the Tab branch (the block
that sets self.log_tab, self.log_scroll and returns vec![]) so it does not run
when self.screen is Screen::Health (in addition to excluding
Screen::UserSearch). Locate the Tab branch in app.rs (the code that checks
key.code == KeyCode::Tab && self.show_logs ...) and add a check like &&
!matches!(self.screen, Screen::Health) (or equivalent) so Tab will reach
handle_health_key() when on the Health screen.
---
Nitpick comments:
In `@src/screen/group_detail.rs`:
- Around line 174-186: The two similar functions draw_relays in group_detail.rs
and profile.rs should be consolidated into a single helper that accepts a
unified relay input (e.g., an iterator or slice of &str/impl AsRef<str> or a
Vec<String>) and returns or renders the same Paragraph/lines; update
group_detail::draw_relays and profile::draw_relays to call this new helper (or
convert their input Value/String arrays to a Vec<&str> and pass it in). Locate
the existing functions named draw_relays and extract the Span/Line construction
and frame.render_widget call into a shared function (e.g., render_relays or
build_relay_paragraph) so both modules reuse the same rendering logic while
preserving styling (Style::default().fg(Color::DarkGray) and Color::White).
In `@src/widget/message_list.rs`:
- Around line 481-489: Change visible_range to return Option<(usize, usize)>
instead of (usize, usize): call self.compute_visible(area) as before, and if
visible.first()/visible.last() yield indices return Some((first, last)),
otherwise return None. Update the function signature visible_range(&self, area:
Rect) -> Option<(usize, usize)> and adjust any callers that expect (0,0) to
handle None (or map None to (0,0) where backward compatibility is required). Use
the same pattern of matching on visible.first() and visible.last() as in the
original implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ee5b56c0-6be2-4e97-9e8c-f456d9e9435c
📒 Files selected for processing (13)
src/action.rssrc/app.rssrc/event.rssrc/main.rssrc/screen/group_detail.rssrc/screen/health.rssrc/screen/main_screen.rssrc/screen/mod.rssrc/screen/profile.rssrc/screen/user_search.rssrc/widget/chat_list.rssrc/widget/message_list.rssrc/wn.rs
jgmontoya
force-pushed
the
feat/health-and-fixes
branch
from
March 11, 2026 19:34
673323b to
42346bc
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/widget/message_list.rs (1)
481-489: Consider clarifying return value semantics for edge cases.
visible_rangereturns(0, 0)both when nothing is visible (empty messages) and when only message 0 is visible. The caller atsrc/screen/main_screen.rs:158assigns this toapp.visible_msg_range.If downstream code needs to distinguish "no messages" from "only message 0 visible", consider returning an
Option<(usize, usize)>or documenting the current behavior. This is minor since the current usage inmain_screen.rsappears to handle both cases equivalently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/widget/message_list.rs` around lines 481 - 489, The current visible_range method conflates "no visible messages" and "only message 0 visible"; change visible_range to return Option<(usize, usize)> (i.e., Some(first,last) when there are visible messages and None when nothing is visible) by calling compute_visible(area) and matching on visible.first()/visible.last() to return Some((first,last)) or None; update any callers (e.g., assignment to app.visible_msg_range in main_screen.rs) to handle the Option (or map/unwrap as appropriate) so semantics are unambiguous—refer to visible_range, compute_visible, and visible_msg_range when making the edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/widget/message_list.rs`:
- Around line 481-489: The current visible_range method conflates "no visible
messages" and "only message 0 visible"; change visible_range to return
Option<(usize, usize)> (i.e., Some(first,last) when there are visible messages
and None when nothing is visible) by calling compute_visible(area) and matching
on visible.first()/visible.last() to return Some((first,last)) or None; update
any callers (e.g., assignment to app.visible_msg_range in main_screen.rs) to
handle the Option (or map/unwrap as appropriate) so semantics are
unambiguous—refer to visible_range, compute_visible, and visible_msg_range when
making the edits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7aa1f335-eaac-4bfa-ad82-de3fc9f4889f
📒 Files selected for processing (8)
src/action.rssrc/app.rssrc/event.rssrc/main.rssrc/screen/health.rssrc/screen/main_screen.rssrc/screen/mod.rssrc/widget/message_list.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/screen/mod.rs
jgmontoya
force-pushed
the
feat/health-and-fixes
branch
from
March 11, 2026 19:42
42346bc to
993d40d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/widget/message_list.rs (1)
583-640:⚠️ Potential issue | 🔴 CriticalUse
compute_visible()for rendering too.
visible_range()andimage_positions()now use the anchor-based algorithm, butrender()still uses the old bottom-fill logic. That means the new “scroll to top fills forward” behavior only exists in bookkeeping/tests; the UI can still render just the oldest message whenscroll_from_bottom = total - 1, and image placement can drift from what is actually on screen.Suggested fix
- let visible_height = inner.height as usize; let width = inner.width as usize; - let total = self.messages.len(); - let skip_messages = self.scroll_from_bottom.min(total); - - let mut visible_msgs: Vec<usize> = Vec::new(); - let mut used_rows = 0; - - let end = total.saturating_sub(skip_messages); - for i in (0..end).rev() { - let h = message_height_with_images(&self.messages[i], width, self.inline_images); - if used_rows + h > visible_height { - break; - } - used_rows += h; - visible_msgs.push(i); - } - visible_msgs.reverse(); - - // Render each visible message - let mut y = inner.y; - for &idx in &visible_msgs { + let (visible, _) = self.compute_visible(area); + for (idx, y) in visible { let msg = &self.messages[idx]; let lines = format_message( msg, self.my_pubkey, self.media_downloads, self.inline_images, self.messages, ); let h = message_height_with_images(msg, width, self.inline_images); let is_selected = self.selected == Some(idx); - let msg_area = Rect::new(inner.x, y, inner.width, h as u16); + let msg_area = Rect::new(inner.x, y as u16, inner.width, h as u16); Paragraph::new(lines) .wrap(Wrap { trim: false }) .render(msg_area, buf); if is_selected { for row in msg_area.y..msg_area.y + msg_area.height { for col in msg_area.x..msg_area.x + msg_area.width { if let Some(cell) = buf.cell_mut((col, row)) { cell.set_bg(Color::DarkGray); if cell.fg == Color::DarkGray { cell.set_fg(Color::Gray); } } } } } - - y += h as u16; - if y >= inner.y + inner.height { - break; - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/widget/message_list.rs` around lines 583 - 640, The render() function still computes visible_msgs with the old bottom-fill loop causing UI mismatch; replace that manual visibility calculation with a call to compute_visible() (the same anchor-based helper used by visible_range() and image_positions()) to get the visible indices and their layout info, then iterate those returned indices/heights/positions to build Paragraphs and to apply selection highlighting so image placement and selection match the bookkeeping/tests (use compute_visible() instead of the manual loop that calls message_height_with_images and builds visible_msgs; keep using format_message/message_height_with_images for content and heights but derive ordering and y offsets from compute_visible()).
🧹 Nitpick comments (1)
src/app.rs (1)
163-164: Consider type consistency between relay collections.
group_relaysisVec<String>whileaccount_relaysisVec<Value>. If both represent relay URLs, using the same type would improve consistency and reduce cognitive load.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` around lines 163 - 164, The struct defines group_relays: Vec<String> but account_relays: Vec<Value>, causing inconsistent types for relay URL collections; change account_relays to the same concrete type (e.g., Vec<String>) or convert group_relays to Vec<Value> consistently: locate the struct fields named group_relays and account_relays and replace the account_relays type with Vec<String> (or vice‑versa) and update any parsing/serialization code that constructs or consumes account_relays (e.g., JSON deserialization, serde annotations, and uses of account_relays elsewhere) to use the unified type so both relay collections share the same representation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.rs`:
- Around line 517-540: The handlers for Effect::LoadGroupRelays and
Effect::LoadAccountRelays currently swallow wn::exec errors or malformed JSON by
returning empty vecs; update them to mirror the LoadRelayHealth pattern by
matching wn::exec result and on Err or unexpected serde_json shape send a
distinct error action (e.g. Event::Action(Action::GroupRelaysLoadFailed(...))
and Event::Action(Action::AccountRelaysLoadFailed(...))) instead of
Event::Action(Action::GroupRelaysLoaded(vec![]))/AccountRelaysLoaded(vec![]);
preserve the successful path that sends GroupRelaysLoaded and
AccountRelaysLoaded with parsed Vec<String> but on any error include the error
details in the failure action so the UI can surface it.
In `@src/screen/health.rs`:
- Around line 473-487: The current early-return treats
app.relay_health.is_none() as only “loading”; update the branch that renders the
loading Paragraph to first check for an error emitted by the effect
(Action::RelayHealthError) and render an error message when present, otherwise
show the loading spinner; if there is a stored/stale value (e.g.
app.relay_health is Some but an error flag exists) render the stale data instead
of the spinner, and avoid returning early when you need to show error or stale
UI — adjust the code around the existing app.relay_health check and the loading
Paragraph to consult the error state (the error field populated by
Action::RelayHealthError) and render accordingly.
---
Outside diff comments:
In `@src/widget/message_list.rs`:
- Around line 583-640: The render() function still computes visible_msgs with
the old bottom-fill loop causing UI mismatch; replace that manual visibility
calculation with a call to compute_visible() (the same anchor-based helper used
by visible_range() and image_positions()) to get the visible indices and their
layout info, then iterate those returned indices/heights/positions to build
Paragraphs and to apply selection highlighting so image placement and selection
match the bookkeeping/tests (use compute_visible() instead of the manual loop
that calls message_height_with_images and builds visible_msgs; keep using
format_message/message_height_with_images for content and heights but derive
ordering and y offsets from compute_visible()).
---
Nitpick comments:
In `@src/app.rs`:
- Around line 163-164: The struct defines group_relays: Vec<String> but
account_relays: Vec<Value>, causing inconsistent types for relay URL
collections; change account_relays to the same concrete type (e.g., Vec<String>)
or convert group_relays to Vec<Value> consistently: locate the struct fields
named group_relays and account_relays and replace the account_relays type with
Vec<String> (or vice‑versa) and update any parsing/serialization code that
constructs or consumes account_relays (e.g., JSON deserialization, serde
annotations, and uses of account_relays elsewhere) to use the unified type so
both relay collections share the same representation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09ba1a23-9b4f-4f0d-87e4-e41a1d52365f
📒 Files selected for processing (8)
src/action.rssrc/app.rssrc/event.rssrc/main.rssrc/screen/health.rssrc/screen/main_screen.rssrc/screen/mod.rssrc/widget/message_list.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/event.rs
- src/screen/main_screen.rs
jgmontoya
force-pushed
the
feat/health-and-fixes
branch
from
March 11, 2026 20:01
993d40d to
369862b
Compare
dannym-arx
reviewed
Mar 11, 2026
dannym-arx
left a comment
dannym-arx
left a comment
Contributor
There was a problem hiding this comment.
three minor comments by marmy
jgmontoya
force-pushed
the
feat/health-and-fixes
branch
from
March 11, 2026 20:08
369862b to
2ea324f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/widget/message_list.rs (1)
583-600:⚠️ Potential issue | 🟠 MajorRender from
compute_visible()instead of re-derivingvisible_msgs.Lines 433-489 moved visibility to the anchor-based
compute_visible()path, but this block still uses the old reverse-fill algorithm. At the top of the list (scroll_from_bottom == total - 1) or around oversized messages, the messages you paint no longer matchvisible_range()orimage_positions(). Line 158 insrc/screen/main_screen.rsnow stores that range, and Line 159 uses the same helper for inline image rectangles, so both can drift from what is actually on screen.🔧 Suggested direction
- let visible_height = inner.height as usize; let width = inner.width as usize; - let total = self.messages.len(); - let skip_messages = self.scroll_from_bottom.min(total); - - let mut visible_msgs: Vec<usize> = Vec::new(); - let mut used_rows = 0; - - let end = total.saturating_sub(skip_messages); - for i in (0..end).rev() { - let h = message_height_with_images(&self.messages[i], width, self.inline_images); - if used_rows + h > visible_height { - break; - } - used_rows += h; - visible_msgs.push(i); - } - visible_msgs.reverse(); + let (visible_msgs, _) = self.compute_visible(area); - let mut y = inner.y; - for &idx in &visible_msgs { + for (idx, y) in visible_msgs { let msg = &self.messages[idx]; let lines = format_message( msg, @@ - let msg_area = Rect::new(inner.x, y, inner.width, h as u16); + let msg_area = Rect::new(inner.x, y as u16, inner.width, h as u16); @@ - y += h as u16; - if y >= inner.y + inner.height { - break; - } }Also applies to: 603-640
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/widget/message_list.rs` around lines 583 - 600, The painting logic currently re-derives visible_msgs with a reverse-fill loop (using message_height_with_images and scroll_from_bottom) which can drift from the anchor-based visibility computed by compute_visible()/visible_range() and image_positions(); replace this manual loop with the anchor-based API: call the same compute_visible()/visible_range() used elsewhere to obtain the exact visible index range (and pass the same parameters you use for image_positions()), iterate that range to build your visible_msgs (or use the range directly), and remove the reverse-fill logic so both message rendering and inline image rects share the identical visibility computation.src/main.rs (1)
53-81:⚠️ Potential issue | 🟠 MajorAborting streams without cleanup actions will leave loading state stuck.
handle.abort()prevents the epilogue from running at lines 302, 334, 373, and 959, soNotificationStreamEnded,ChatStreamEnded,MessageStreamEnded, andSearchStreamEndedactions are never sent. The original review comment is correct:
messages_loading(set true at line 1956): Only cleared viaMessageStreamEndedor explicit logout reset. WhenUnsubscribeMessagesis called at lines 821 and 1966, the abort preventsMessageStreamEndedfrom firing, leavingmessages_loading = trueindefinitely. This locks the UI in a loading state when switching groups (line 1966) or logging out (line 821).
search_loading(set true at line 2318): Only cleared viaSearchStreamEnded, explicitclear_search_results(), or logout reset. However, at line 2274 (search screen exit),clear_search_results()is called beforeUnsubscribeSearch, which masks the bug by explicitly settingsearch_loading = false. This pattern should be applied to message unsubscription but currently is not.
connected(set true at line 307): Set false only viaChatStreamEndedor logout reset. During re-subscription cycles (whenSubscribeChatscallskill_chats()before spawning the new task), the abort skips the cleanup action, potentially leaving the connection state inconsistent.The fix requires either emitting cleanup actions when unsubscribing (emulate
MessageStreamEndedin theUnsubscribeMessageshandler) or clearing state before the abort, as the search screen already does correctly at line 2274.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 53 - 81, The kill_* helpers (kill_messages, kill_chats, kill_notifications, kill_search) call handle.abort() which prevents the epilogue from dispatching MessageStreamEnded, ChatStreamEnded, NotificationStreamEnded and SearchStreamEnded, leaving messages_loading, search_loading and connected stuck; fix by ensuring state cleanup before abort: in the UnsubscribeMessages/UnsubscribeSearch handlers (and where SubscribeChats calls kill_chats) either dispatch the corresponding *StreamEnded action (MessageStreamEnded, SearchStreamEnded, ChatStreamEnded) or explicitly clear messages_loading, search_loading and connected flags before calling kill_messages/kill_search/kill_chats (mirror the clear_search_results() pattern used at the search exit). Ensure the change touches the kill_* call sites (UnsubscribeMessages, UnsubscribeSearch, SubscribeChats and logout/unsubscribe flows) so the UI flags are always cleared even if the task is aborted.
♻️ Duplicate comments (2)
src/main.rs (1)
517-540:⚠️ Potential issue | 🟠 MajorDon't collapse relay-load failures into empty results.
Both loaders convert CLI errors and unexpected JSON shapes into
...Loaded([]). That makes backend failures indistinguishable from “this account/group has no relays”, so the UI can silently hide broken data. TheLoadRelayHealthbranch on Lines 1109-1116 already uses an explicit error action; these paths should mirror that behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 517 - 540, The two branches Effect::LoadGroupRelays and Effect::LoadAccountRelays currently convert any wn::exec error or non-array JSON into an empty vec and send GroupRelaysLoaded/AccountRelaysLoaded([]); instead, detect wn::exec errors and unexpected JSON shapes and send the same explicit error action used by the LoadRelayHealth branch (mirror its error handling) rather than returning an empty list—i.e., when wn::exec returns Err(_) or the JSON is not an array, construct and tx.send the error Event::Action variant that LoadRelayHealth uses, otherwise map the array to relays and send GroupRelaysLoaded/AccountRelaysLoaded with the successful data.src/app.rs (1)
2242-2244:⚠️ Potential issue | 🟡 MinorClamp Health scrolling to the available content.
health_scrollstill grows without an upper bound here.src/screen/health.rsfeeds that value straight intoParagraph::scroll, so once the user goes past the last rendered line the pane turns blank instead of stopping at the bottom. Please cap it from the actual content height/viewport size when relay data is present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` around lines 2242 - 2244, The health_scroll increment allows scrolling past available content; change the KeyCode::Char('j') | KeyCode::Down handler to compute a max_scroll from the rendered health content height minus the viewport height (use the same logic/data used in src/screen/health.rs that feeds Paragraph::scroll), then set self.health_scroll = (self.health_scroll.saturating_add(1)).min(max_scroll) (or clamp to 0 when no relay data) so health_scroll never exceeds the content bounds and the pane stops at the bottom.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app.rs`:
- Around line 1597-1602: The Health screen branches (e.g., the
KeyCode::Char('h') handler that sets Screen::Health and returns
Effect::LoadRelayHealth) and the retry handler must clear the previous error
state first; before setting self.relay_health = None and returning
Effect::LoadRelayHealth, set self.health_error = None so the UI doesn't show the
stale failure while a new LoadRelayHealth is in flight—apply the same change in
the retry path (the branch around the retry dispatch at the other location).
---
Outside diff comments:
In `@src/main.rs`:
- Around line 53-81: The kill_* helpers (kill_messages, kill_chats,
kill_notifications, kill_search) call handle.abort() which prevents the epilogue
from dispatching MessageStreamEnded, ChatStreamEnded, NotificationStreamEnded
and SearchStreamEnded, leaving messages_loading, search_loading and connected
stuck; fix by ensuring state cleanup before abort: in the
UnsubscribeMessages/UnsubscribeSearch handlers (and where SubscribeChats calls
kill_chats) either dispatch the corresponding *StreamEnded action
(MessageStreamEnded, SearchStreamEnded, ChatStreamEnded) or explicitly clear
messages_loading, search_loading and connected flags before calling
kill_messages/kill_search/kill_chats (mirror the clear_search_results() pattern
used at the search exit). Ensure the change touches the kill_* call sites
(UnsubscribeMessages, UnsubscribeSearch, SubscribeChats and logout/unsubscribe
flows) so the UI flags are always cleared even if the task is aborted.
In `@src/widget/message_list.rs`:
- Around line 583-600: The painting logic currently re-derives visible_msgs with
a reverse-fill loop (using message_height_with_images and scroll_from_bottom)
which can drift from the anchor-based visibility computed by
compute_visible()/visible_range() and image_positions(); replace this manual
loop with the anchor-based API: call the same compute_visible()/visible_range()
used elsewhere to obtain the exact visible index range (and pass the same
parameters you use for image_positions()), iterate that range to build your
visible_msgs (or use the range directly), and remove the reverse-fill logic so
both message rendering and inline image rects share the identical visibility
computation.
---
Duplicate comments:
In `@src/app.rs`:
- Around line 2242-2244: The health_scroll increment allows scrolling past
available content; change the KeyCode::Char('j') | KeyCode::Down handler to
compute a max_scroll from the rendered health content height minus the viewport
height (use the same logic/data used in src/screen/health.rs that feeds
Paragraph::scroll), then set self.health_scroll =
(self.health_scroll.saturating_add(1)).min(max_scroll) (or clamp to 0 when no
relay data) so health_scroll never exceeds the content bounds and the pane stops
at the bottom.
In `@src/main.rs`:
- Around line 517-540: The two branches Effect::LoadGroupRelays and
Effect::LoadAccountRelays currently convert any wn::exec error or non-array JSON
into an empty vec and send GroupRelaysLoaded/AccountRelaysLoaded([]); instead,
detect wn::exec errors and unexpected JSON shapes and send the same explicit
error action used by the LoadRelayHealth branch (mirror its error handling)
rather than returning an empty list—i.e., when wn::exec returns Err(_) or the
JSON is not an array, construct and tx.send the error Event::Action variant that
LoadRelayHealth uses, otherwise map the array to relays and send
GroupRelaysLoaded/AccountRelaysLoaded with the successful data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: beee7b72-d986-405a-b173-0b8980a42c2a
📒 Files selected for processing (8)
src/action.rssrc/app.rssrc/event.rssrc/main.rssrc/screen/health.rssrc/screen/main_screen.rssrc/screen/mod.rssrc/widget/message_list.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/screen/mod.rs
Add a dedicated health screen (`h` from main) showing relay connection status across all planes (inbox, discovery, group, ephemeral). Two views toggled with Tab: "By Status" groups relays by connection state with color-coded indicators, "By Plane" breaks down per-plane stats including subscription counts, router contexts, and watched users. Relays are deduplicated across planes with worst-status-wins semantics and rendered in responsive multi-column layout. Also cleans up dead code: removes unused SettingsUpdateSuccess action, UpdateSetting effect, selected_setting field, max_scroll() helper, and simplifies Event::Resize to drop unused dimensions.
jgmontoya
force-pushed
the
feat/health-and-fixes
branch
from
March 11, 2026 20:15
2ea324f to
ce40ca6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/widget/message_list.rs (1)
433-489:⚠️ Potential issue | 🟠 MajorUnify
render()with the new anchor-based visibility path.Line 433 introduces a new visibility model, but
render()still uses a separate older algorithm. That can desync rendered rows fromvisible_range()/image_positions()(top-scroll and oversized-message cases).🔧 Proposed fix
impl Widget for MessageListWidget<'_> { fn render(self, area: Rect, buf: &mut Buffer) { @@ - let visible_height = inner.height as usize; - let width = inner.width as usize; - let total = self.messages.len(); - let skip_messages = self.scroll_from_bottom.min(total); - - let mut visible_msgs: Vec<usize> = Vec::new(); - let mut used_rows = 0; - - let end = total.saturating_sub(skip_messages); - for i in (0..end).rev() { - let h = message_height_with_images(&self.messages[i], width, self.inline_images); - if used_rows + h > visible_height { - break; - } - used_rows += h; - visible_msgs.push(i); - } - visible_msgs.reverse(); + let width = inner.width as usize; + let (visible, _) = self.compute_visible(area); // Render each visible message - let mut y = inner.y; - for &idx in &visible_msgs { + for (idx, y0) in visible { let msg = &self.messages[idx]; let lines = format_message( msg, @@ ); let h = message_height_with_images(msg, width, self.inline_images); let is_selected = self.selected == Some(idx); - let msg_area = Rect::new(inner.x, y, inner.width, h as u16); + let msg_area = Rect::new(inner.x, y0 as u16, inner.width, h as u16); @@ - y += h as u16; - if y >= inner.y + inner.height { - break; - } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/widget/message_list.rs` around lines 433 - 489, render() is still using the old visibility algorithm and must be changed to use compute_visible() so rendered rows match visible_range() and image_positions(); call compute_visible(area) inside render(), use the returned Vec<(usize,usize)> to iterate messages and render each message by its index and y position (instead of independently recomputing start/anchor), and update image_positions() to derive positions from compute_visible() as well so top-scroll and oversized-message cases follow the anchor-based layout exactly (refer to compute_visible, render, visible_range, image_positions).
♻️ Duplicate comments (1)
src/main.rs (1)
517-540:⚠️ Potential issue | 🟠 MajorDon’t collapse relay-load failures into empty results.
Both loaders still treat errors/unexpected shapes as “no relays,” which hides backend/CLI failures and can silently mislead the UI.
🔧 Proposed fix
Effect::LoadGroupRelays { account, group_id } => { let tx = tx.clone(); tokio::spawn(async move { - let relays = - match wn::exec(&["--account", &account, "groups", "relays", &group_id]).await { - Ok(serde_json::Value::Array(arr)) => arr - .into_iter() - .filter_map(|v| v.as_str().map(String::from)) - .collect(), - _ => vec![], - }; - let _ = tx.send(Event::Action(Action::GroupRelaysLoaded(relays))); + let action = match wn::exec(&["--account", &account, "groups", "relays", &group_id]).await { + Ok(serde_json::Value::Array(arr)) => { + let relays = arr.into_iter().filter_map(|v| v.as_str().map(String::from)).collect(); + Action::GroupRelaysLoaded(relays) + } + Ok(other) => Action::GroupRelaysLoadFailed(format!("Unexpected payload: {other}")), + Err(e) => Action::GroupRelaysLoadFailed(e.to_string()), + }; + let _ = tx.send(Event::Action(action)); }); } Effect::LoadAccountRelays { account } => { let tx = tx.clone(); tokio::spawn(async move { - let relays = match wn::exec(&["--account", &account, "relays", "list"]).await { - Ok(serde_json::Value::Array(arr)) => arr, - _ => vec![], - }; - let _ = tx.send(Event::Action(Action::AccountRelaysLoaded(relays))); + let action = match wn::exec(&["--account", &account, "relays", "list"]).await { + Ok(serde_json::Value::Array(arr)) => Action::AccountRelaysLoaded(arr), + Ok(other) => Action::AccountRelaysLoadFailed(format!("Unexpected payload: {other}")), + Err(e) => Action::AccountRelaysLoadFailed(e.to_string()), + }; + let _ = tx.send(Event::Action(action)); }); }// src/action.rs (additions) GroupRelaysLoadFailed(String), AccountRelaysLoadFailed(String),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 517 - 540, The loaders currently swallow wn::exec failures by returning empty vecs; update Effect::LoadGroupRelays and Effect::LoadAccountRelays to inspect the Result from wn::exec and on Err or unexpected JSON shape send a failure action event (e.g. Event::Action(Action::GroupRelaysLoadFailed(err_msg)) and Event::Action(Action::AccountRelaysLoadFailed(err_msg))) instead of returning an empty Vec; keep using wn::exec, but on Ok(...) still parse the Array into the expected Vec<String> (like the existing filter_map for group relays) and send GroupRelaysLoaded/AccountRelaysLoaded on success, and on any other branch convert the error or unexpected format into a descriptive String (error.to_string() or a custom message) and send the new failure events (GroupRelaysLoadFailed, AccountRelaysLoadFailed) so callers can distinguish backend/CLI failures from legitimately empty relay lists.
🧹 Nitpick comments (1)
src/screen/health.rs (1)
49-128: Consider consolidating relay traversal logic.
extract_relaysandextract_plane_relaysduplicate the same tree walk with slightly different outputs. A shared iterator/helper would reduce drift risk over time.Also applies to: 131-196
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screen/health.rs` around lines 49 - 128, The relay-traversal logic is duplicated between extract_relays and extract_plane_relays; create a single helper (e.g., a function iterate_relays or collect_all_relays) that encapsulates the tree walk currently done in extract_relays (and mirrored in extract_plane_relays) and yields a unified stream/Vec of (url, status, section) tuples; keep using collect_from_session (or fold its logic into the new helper), then have extract_relays consume that iterator/Vec to build the BTreeMap, apply status_order and construct RelayEntry; update extract_plane_relays to reuse the same helper to avoid drift while keeping symbols status_order, RelayEntry, collect_from_session, extract_relays and extract_plane_relays intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app.rs`:
- Around line 1559-1571: When switching to Screen::Profile you already set
profile=None, selected_follow=0 and follows_loading=true but you must also clear
any stale collections so UI/actions don't operate on old data; update the same
branch (the block that sets self.screen = Screen::Profile) to also set
self.follows = Vec::new() and self.account_relays = Default::default() (or the
appropriate empty collection type used for account_relays) before emitting the
LoadProfile/LoadFollows/LoadAccountRelays effects so the profile view and follow
actions cannot see or act on previous visit data.
- Around line 483-488: The GroupRelaysLoaded and AccountRelaysLoaded handlers
currently overwrite self.group_relays and self.account_relays unconditionally,
allowing stale async responses to clobber the currently-viewed data; modify the
Action variants (Action::GroupRelaysLoaded and Action::AccountRelaysLoaded) to
carry the corresponding identity (e.g., group_id / account id) along with the
relays, and in the handlers compare the incoming identity to the current
selected group/account before assigning to self.group_relays or
self.account_relays; also update the producer/fetch side that constructs these
actions so it threads the same identity through the response action.
---
Outside diff comments:
In `@src/widget/message_list.rs`:
- Around line 433-489: render() is still using the old visibility algorithm and
must be changed to use compute_visible() so rendered rows match visible_range()
and image_positions(); call compute_visible(area) inside render(), use the
returned Vec<(usize,usize)> to iterate messages and render each message by its
index and y position (instead of independently recomputing start/anchor), and
update image_positions() to derive positions from compute_visible() as well so
top-scroll and oversized-message cases follow the anchor-based layout exactly
(refer to compute_visible, render, visible_range, image_positions).
---
Duplicate comments:
In `@src/main.rs`:
- Around line 517-540: The loaders currently swallow wn::exec failures by
returning empty vecs; update Effect::LoadGroupRelays and
Effect::LoadAccountRelays to inspect the Result from wn::exec and on Err or
unexpected JSON shape send a failure action event (e.g.
Event::Action(Action::GroupRelaysLoadFailed(err_msg)) and
Event::Action(Action::AccountRelaysLoadFailed(err_msg))) instead of returning an
empty Vec; keep using wn::exec, but on Ok(...) still parse the Array into the
expected Vec<String> (like the existing filter_map for group relays) and send
GroupRelaysLoaded/AccountRelaysLoaded on success, and on any other branch
convert the error or unexpected format into a descriptive String
(error.to_string() or a custom message) and send the new failure events
(GroupRelaysLoadFailed, AccountRelaysLoadFailed) so callers can distinguish
backend/CLI failures from legitimately empty relay lists.
---
Nitpick comments:
In `@src/screen/health.rs`:
- Around line 49-128: The relay-traversal logic is duplicated between
extract_relays and extract_plane_relays; create a single helper (e.g., a
function iterate_relays or collect_all_relays) that encapsulates the tree walk
currently done in extract_relays (and mirrored in extract_plane_relays) and
yields a unified stream/Vec of (url, status, section) tuples; keep using
collect_from_session (or fold its logic into the new helper), then have
extract_relays consume that iterator/Vec to build the BTreeMap, apply
status_order and construct RelayEntry; update extract_plane_relays to reuse the
same helper to avoid drift while keeping symbols status_order, RelayEntry,
collect_from_session, extract_relays and extract_plane_relays intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c7dd81f-35fe-4e13-b59d-5fb4ef15a9db
📒 Files selected for processing (8)
src/action.rssrc/app.rssrc/event.rssrc/main.rssrc/screen/health.rssrc/screen/main_screen.rssrc/screen/mod.rssrc/widget/message_list.rs
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
erskingardner
approved these changes
Mar 12, 2026
erskingardner
left a comment
erskingardner
left a comment
Member
There was a problem hiding this comment.
My agent found a few small things but they all seem to be outside of the diff range, so LGTM
dannym-arx
approved these changes
Mar 12, 2026
mubarakcoded
approved these changes
Mar 12, 2026
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
4 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.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes