refactor(services): make AsyncState the single async-UI code path

farhan-syah · farhan-syah · commit b975e8cb0e69 · 2026-06-14T11:27:06.000+08:00
The AsyncState/from_value/IsEmpty primitive shipped behind #[allow(dead_code)]
with zero non-test consumers: the notification popover re-implemented the four
state arms inline instead of calling from_value, so the unit tests validated a
mapping the render path never ran. Worse, the two diverged — from_value derived
Empty from the raw feed while the popover derived it from the capability-filtered
list — so the tests gave false confidence about the live behavior.

Root cause was StudioError not being Clone, which blocked owning a Resource read
out of its guard and forced the inline copy. NodeDbError is Clone, so:

- Derive Clone on StudioError (cheap, lossless) so a resource value can be cloned
  out of its guard and mapped through from_value.
- Drop the dead_code allows on AsyncState/IsEmpty/from_value — now genuinely used.
- Add is_loading/is_empty/error_message/is_retriable/loaded accessors so views
  drive AsyncView from one AsyncState without re-deriving flags by hand.
- Rewrite the notification popover to map the Ok payload to the capability-
  filtered list BEFORE from_value, then render via the accessors. The render path
  and the unit tests now share one code path, and Empty correctly reflects what
  the active connection can see.

fmt + clippy -D warnings + nextest all green (31 tests, was 27).
diff --git a/nodedb-studio/src/components/async_view.rs b/nodedb-studio/src/components/async_view.rs
@@ -5,24 +5,24 @@
 //! states to this component. Keeps every wired view from re-implementing the
 //! spinner / empty / error+retry markup.
 //!
-//! Calling convention (wired up by plan 01-04):
+//! Calling convention: build one `AsyncState<T>` via `from_value` and drive both
+//! this component and the loaded markup from it — no inline match arms:
 //! ```ignore
-//! match AsyncState::from_value(resource.read().clone()) {
-//!     AsyncState::Loaded(data) => rsx! { /* caller's own markup for `data` */ },
-//!     other => rsx! {
-//!         AsyncView {
-//!             loading: matches!(other, AsyncState::Loading),
-//!             empty: matches!(other, AsyncState::Empty),
-//!             error: match &other { AsyncState::Error(e) => Some(e.to_string()), _ => None },
-//!             retriable: matches!(&other, AsyncState::Error(e) if e.is_retriable()),
-//!             on_retry: move |_| resource.restart(),
-//!         }
+//! let state = AsyncState::from_value(resource.read().clone());
+//! rsx! {
+//!     AsyncView {
+//!         loading: state.is_loading(),
+//!         empty: state.is_empty(),
+//!         error: state.error_message(),
+//!         retriable: state.is_retriable(),
+//!         on_retry: move |_| resource.restart(),
 //!     }
+//!     if let Some(data) = state.loaded() { /* caller's own markup for `data` */ }
 //! }
 //! ```
 //! Discrete props (rather than the generic `AsyncState<T>`) are used because
-//! Dioxus props must be `Clone + PartialEq`; `StudioError` is neither, and `T`
-//! is unconstrained — so the caller decodes the state and passes flags.
+//! Dioxus props must be `Clone + PartialEq`; `StudioError`/`T` need not be — so
+//! the caller decodes the state via the accessors and passes plain flags.
 
 use dioxus::prelude::*;
 
diff --git a/nodedb-studio/src/components/popovers/notification_popover.rs b/nodedb-studio/src/components/popovers/notification_popover.rs
@@ -1,22 +1,17 @@
 //! Notification popover: capability-filtered, grouped list with a bell badge,
 //! mark-all-read, and click-to-navigate.
 //!
-//! SEAM-04 render-path proof: the rendered list is fetched through the async
-//! `ConnectionService` seam via `use_resource` (reading `Rc<dyn ConnectionService>`
-//! from context), and the four UI states are surfaced exactly as the
-//! `AsyncState`/`AsyncView` primitive (01-03) prescribes — Loading / Empty /
-//! Error render via the shared `AsyncView`, Loaded renders the existing grouped
-//! list. The Error state offers a Retry gated on `StudioError::is_retriable()`,
-//! wired to `Resource::restart()`.
+//! The rendered list is fetched through the async `ConnectionService` seam via
+//! `use_resource`, then mapped to `AsyncState` and rendered through the shared
+//! `AsyncView` — the canonical async-UI pattern. Capability filtering is applied
+//! to the `Ok` payload *before* `AsyncState::from_value`, so `Empty` reflects
+//! what THIS connection can see (not the raw feed). The Error state offers a
+//! Retry gated on `StudioError::is_retriable()`, wired to `Resource::restart()`.
 //!
-//! Reconciliation with 01-02's app.rs seeding: plan 01-02 seeds a global
-//! `Signal<Vec<Notification>>` that `topbar.rs` reads for the unread bell badge.
-//! That signal and the topbar badge are LEFT UNCHANGED here — the badge keeps
-//! deriving from the app.rs-seeded signal, and mark-all-read / per-item clicks
-//! still mutate it so the badge stays in sync. The popover ADDITIONALLY
-//! self-fetches via `use_resource` as the live proof of the async pattern. The
-//! minor redundancy (badge from signal, list from resource) is intentional and
-//! acceptable for Phase 1; later phases converge the two onto one source.
+//! The topbar bell badge derives from a separate global `Signal<Vec<Notification>>`
+//! seeded in `app.rs`; mark-all-read / per-item clicks mutate that signal so the
+//! badge stays in sync. Converging the badge and this list onto one source is
+//! deferred to a later phase.
 
 use std::rc::Rc;
 
@@ -25,6 +20,7 @@ use dioxus::prelude::*;
 use crate::components::async_view::AsyncView;
 use crate::models::notification::{Notification, NotificationTarget};
 use crate::routes::Route;
+use crate::services::async_state::AsyncState;
 use crate::services::connection_service::ConnectionService;
 use crate::state::connection::{ActiveConnection, Capabilities};
 use crate::state::notifications::visible;
@@ -62,47 +58,30 @@ pub fn NotificationPopover() -> Element {
         None => return rsx! {},
     };
 
-    // SEAM-04 PROOF: fetch the feed through the async seam. Clone the Rc BEFORE
-    // the async block; never hold a signal/Resource guard across `.await`.
+    // Fetch the feed through the async seam. Clone the Rc BEFORE the async block;
+    // never hold a signal/Resource guard across `.await`.
     let mut feed = use_resource(move || {
         let service = service.clone();
         async move { service.notifications().await } // Result<Vec<Notification>, StudioError>
     });
 
-    // Map the resource read -> the four AsyncState states (mirrors
-    // AsyncState::from_value: None->Loading, Some(Err)->Error,
-    // Some(Ok(empty))->Empty, Some(Ok(data))->Loaded). StudioError is not Clone,
-    // so derive (message, retriable) by reference while the guard is held, clone
-    // ONLY the Ok Vec (Notification is Clone), then DROP the guard before render.
-    let mut loading = false;
-    let mut empty = false;
-    let mut error_msg: Option<String> = None;
-    let mut retriable = false;
-    let mut loaded: Option<Vec<Notification>> = None;
-    {
-        let read = feed.read();
-        match &*read {
-            None => loading = true,
-            Some(Err(e)) => {
-                error_msg = Some(e.to_string()); // Display
-                retriable = e.is_retriable(); // gates Retry
-            }
-            Some(Ok(list)) => {
-                // Capability-filter here so Empty reflects what THIS connection sees.
-                let vis: Vec<Notification> = visible(&list[..], &caps).cloned().collect();
-                if vis.is_empty() {
-                    empty = true;
-                } else {
-                    loaded = Some(vis);
-                }
-            }
-        }
-    } // guard dropped here — nothing held across the render below
+    // Clone the resource value out of its guard (StudioError is Clone), mapping
+    // the Ok payload to the capability-filtered list so `Empty` reflects what THIS
+    // connection sees — then derive every UI state from one `AsyncState`. This is
+    // the same `from_value` mapping the unit tests exercise; the render path and
+    // the tests share one code path.
+    let state: AsyncState<Vec<Notification>> = {
+        let mapped = feed
+            .read()
+            .clone()
+            .map(|res| res.map(|list| visible(&list[..], &caps).cloned().collect::<Vec<_>>()));
+        AsyncState::from_value(mapped)
+    }; // guard dropped here — nothing held across the render below
 
     // Build the grouped list ONLY for the Loaded case (preserves first-seen order).
     let mut groups: Vec<(String, Vec<Notification>)> = Vec::new();
     let mut unread = 0usize;
-    if let Some(ref items) = loaded {
+    if let Some(items) = state.loaded() {
         unread = items.iter().filter(|n| n.unread).count();
         for n in items {
             match groups.iter_mut().find(|(g, _)| g == &n.group) {
@@ -131,17 +110,17 @@ pub fn NotificationPopover() -> Element {
                 }
             }
             div { class: "notif-list",
-                // Loading / Empty / Error -> shared AsyncView (the proof).
+                // Loading / Empty / Error -> shared AsyncView, driven by AsyncState.
                 AsyncView {
-                    loading,
-                    empty,
-                    error: error_msg.clone(),
-                    retriable,
+                    loading: state.is_loading(),
+                    empty: state.is_empty(),
+                    error: state.error_message(),
+                    retriable: state.is_retriable(),
                     empty_message: "No notifications for this connection".to_string(),
                     on_retry: move |_| feed.restart(),
                 }
                 // Loaded -> the existing grouped list markup.
-                if loaded.is_some() {
+                if state.loaded().is_some() {
                     for (group_name, items) in groups {
                         div { class: "notif-group-label", "{group_name}" }
                         for n in items {
@@ -184,7 +163,6 @@ pub fn NotificationPopover() -> Element {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::services::async_state::AsyncState;
     use crate::services::error::StudioError;
 
     #[test]
diff --git a/nodedb-studio/src/services/async_state.rs b/nodedb-studio/src/services/async_state.rs
@@ -8,10 +8,6 @@ use crate::services::error::StudioError;
 
 /// Anything that can report emptiness, so `from_value` can distinguish a
 /// loaded-but-empty result (-> Empty) from a loaded-with-data result.
-// Scoped allow: see the note on `AsyncState` below. `IsEmpty` exists only to
-// bound `from_value`, which is exercised by tests + mirrored (not called) in the
-// popover render path, so the non-test build sees it as unused.
-#[allow(dead_code)]
 pub trait IsEmpty {
     fn is_empty(&self) -> bool;
 }
@@ -22,15 +18,10 @@ impl<T> IsEmpty for Vec<T> {
     }
 }
 
-// Scoped allow: `AsyncState`/`from_value` are the canonical, unit-tested mapping
-// from a `use_resource` read to the four UI states. The 01-04 popover render path
-// uses approach (A) from the plan — it MIRRORS these four match arms inline
-// rather than CALLING `from_value`, because `StudioError` is not `Clone` and
-// cannot be owned out of a `Resource` read guard for the Error arm. `from_value`
-// is therefore referenced only by tests, so the non-test build flags it (and the
-// enum it returns) as dead. The allow is kept deliberately; phases 3-6 that can
-// own their resource values WILL call `from_value` directly, removing the need.
-#[allow(dead_code)]
+/// The canonical, unit-tested mapping from a `use_resource` read to the four UI
+/// states. Wired views call `from_value` directly (cloning the resource value
+/// out of its guard — `StudioError` is `Clone`) and then drive `AsyncView` via
+/// the accessors below, so no view re-implements the match arms inline.
 pub enum AsyncState<T> {
     Loading,
     Empty,
@@ -44,9 +35,9 @@ impl<T: IsEmpty> AsyncState<T> {
     ///   Some(Err(e))      -> Error(e)
     ///   Some(Ok(empty))   -> Empty
     ///   Some(Ok(data))    -> Loaded(data)
-    // Scoped allow: see the note on `AsyncState` above (test-only call site this
-    // phase; the popover mirrors these arms inline because StudioError is not Clone).
-    #[allow(dead_code)]
+    ///
+    /// To make `Empty` reflect a post-filtered view (e.g. capability filtering),
+    /// map the `Ok` payload to the filtered collection *before* calling this.
     pub fn from_value(v: Option<Result<T, StudioError>>) -> Self {
         match v {
             None => AsyncState::Loading,
@@ -57,6 +48,42 @@ impl<T: IsEmpty> AsyncState<T> {
     }
 }
 
+impl<T> AsyncState<T> {
+    /// True while the first fetch is pending. Feeds `AsyncView`'s `loading` prop.
+    pub fn is_loading(&self) -> bool {
+        matches!(self, AsyncState::Loading)
+    }
+
+    /// True when the fetch resolved to no rows. Feeds `AsyncView`'s `empty` prop.
+    pub fn is_empty(&self) -> bool {
+        matches!(self, AsyncState::Empty)
+    }
+
+    /// `Some(message)` when the fetch failed; `None` otherwise. Feeds
+    /// `AsyncView`'s `error` prop (Display string, since `StudioError` is not a
+    /// Dioxus-compatible prop type).
+    pub fn error_message(&self) -> Option<String> {
+        match self {
+            AsyncState::Error(e) => Some(e.to_string()),
+            _ => None,
+        }
+    }
+
+    /// Whether the failed fetch is retriable. Feeds `AsyncView`'s `retriable`
+    /// prop, which gates the Retry button.
+    pub fn is_retriable(&self) -> bool {
+        matches!(self, AsyncState::Error(e) if e.is_retriable())
+    }
+
+    /// The loaded payload, if any — the caller renders its own markup for it.
+    pub fn loaded(&self) -> Option<&T> {
+        match self {
+            AsyncState::Loaded(t) => Some(t),
+            _ => None,
+        }
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -84,4 +111,39 @@ mod tests {
         let s = AsyncState::<Vec<u8>>::from_value(Some(Err(StudioError::NotConnected)));
         assert!(matches!(s, AsyncState::Error(_)));
     }
+
+    #[test]
+    fn accessors_for_loading() {
+        let s = AsyncState::<Vec<u8>>::from_value(None);
+        assert!(s.is_loading());
+        assert!(!s.is_empty());
+        assert_eq!(s.error_message(), None);
+        assert!(!s.is_retriable());
+        assert!(s.loaded().is_none());
+    }
+
+    #[test]
+    fn accessors_for_empty() {
+        let s = AsyncState::from_value(Some(Ok(Vec::<u8>::new())));
+        assert!(s.is_empty());
+        assert!(!s.is_loading());
+        assert!(s.loaded().is_none());
+    }
+
+    #[test]
+    fn accessors_for_loaded() {
+        let s = AsyncState::from_value(Some(Ok(vec![1u8, 2, 3])));
+        assert_eq!(s.loaded(), Some(&vec![1u8, 2, 3]));
+        assert!(!s.is_loading());
+        assert!(!s.is_empty());
+    }
+
+    #[test]
+    fn accessors_for_error() {
+        // `NotConnected` is non-retriable; its Display message is surfaced.
+        let s = AsyncState::<Vec<u8>>::from_value(Some(Err(StudioError::NotConnected)));
+        assert!(s.error_message().is_some());
+        assert!(!s.is_retriable());
+        assert!(!s.is_loading());
+    }
 }
diff --git a/nodedb-studio/src/services/error.rs b/nodedb-studio/src/services/error.rs
@@ -9,8 +9,10 @@ use nodedb_types::error::ErrorDetails;
 use thiserror::Error;
 
 // The seam's Result error: returned by every `ConnectionService` method and
-// consumed by the views in later plans (01-03..04).
-#[derive(Debug, Error)]
+// consumed by the views. `Clone` so a `Resource` read can be cloned out of its
+// guard and mapped through `AsyncState::from_value` (the canonical render path);
+// `NodeDbError` is itself `Clone`, so this is a cheap, lossless derive.
+#[derive(Debug, Clone, Error)]
 pub enum StudioError {
     #[error("connection error: {0}")]
     Connection(#[source] NodeDbError),
