From 71285116932cd99232e8b1cb5c9a074dbd2340f0 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Sat, 27 Jun 2026 14:50:34 -0300 Subject: [PATCH 01/10] refactor(app): remove unused command and transition scaffolding This commit removes placeholder command-dispatch and transition types that were never wired into the App actor loop. AppMessage is trimmed to the variants the runtime actually uses, simplifying the actor control path ahead of the AppHandle surface cleanup in the follow-up commit. This commit is part of the architecture's refactoring phase 13. Signed-off-by: lorenzoberts --- src/app/actor.rs | 2 +- src/app/commands.rs | 23 ----------------------- src/app/messages.rs | 12 +----------- src/app/mod.rs | 2 -- src/app/transitions.rs | 10 ---------- 5 files changed, 2 insertions(+), 47 deletions(-) delete mode 100644 src/app/commands.rs delete mode 100644 src/app/transitions.rs diff --git a/src/app/actor.rs b/src/app/actor.rs index 69475c0..2c8504e 100644 --- a/src/app/actor.rs +++ b/src/app/actor.rs @@ -189,7 +189,7 @@ impl AppActor { reply_to.send(self.app.input_context()).ok(); Outcome::Continue } - Some(AppMessage::Input { .. }) | None => Outcome::Continue, + None => Outcome::Continue, } } None => Outcome::Continue, diff --git a/src/app/commands.rs b/src/app/commands.rs deleted file mode 100644 index 12d5680..0000000 --- a/src/app/commands.rs +++ /dev/null @@ -1,23 +0,0 @@ -//! High-level application intents. Intended to become the App actor message -//! protocol in later phases; most handlers still call `App` methods directly. - -use crate::{app::screens::CurrentScreen, lore::domain::patch::Patch}; - -/// Sub-actions performed during [`crate::app::App::consolidate_patchset_actions`]. -#[allow(dead_code)] // reserved for future command dispatch -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum PatchsetCommand { - SyncBookmark, - ReplyWithReviewedBy, - Apply, -} - -/// Top-level commands the application may process (placeholder for future wiring). -#[derive(Debug, Clone)] -#[allow(dead_code)] -pub enum AppCommand { - NavigateTo(CurrentScreen), - SelectMailingList(String), - FetchNextPage, - OpenPatchset(Patch), -} diff --git a/src/app/messages.rs b/src/app/messages.rs index 50bb445..d9f883a 100644 --- a/src/app/messages.rs +++ b/src/app/messages.rs @@ -1,10 +1,8 @@ -use std::ops::ControlFlow; - use tokio::sync::oneshot; use crate::{ app::{errors::AppError, view_model::AppViewModel}, - input::{context::InputContext, event::InputEvent}, + input::context::InputContext, }; /// Typed message protocol for the `AppActor`. @@ -17,13 +15,6 @@ pub enum AppMessage { reply_to: oneshot::Sender>, }, - /// Inject a synthetic input event for processing. - #[allow(dead_code)] - Input { - event: InputEvent, - reply_to: oneshot::Sender>>, - }, - /// Request a snapshot of the current presentation model. GetViewModel { reply_to: oneshot::Sender, @@ -42,7 +33,6 @@ impl AppMessage { pub fn name(&self) -> &'static str { match self { AppMessage::Initialize { .. } => "Initialize", - AppMessage::Input { .. } => "Input", AppMessage::GetViewModel { .. } => "GetViewModel", AppMessage::GetInputContext { .. } => "GetInputContext", AppMessage::Shutdown { .. } => "Shutdown", diff --git a/src/app/mod.rs b/src/app/mod.rs index 78d1f00..8b7b8e3 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -1,5 +1,4 @@ pub mod actor; -pub mod commands; pub mod errors; pub(crate) mod flows; pub mod handle; @@ -9,7 +8,6 @@ pub mod messages; pub mod popup; pub mod screens; pub mod state; -pub mod transitions; pub mod updates; pub mod view_model; diff --git a/src/app/transitions.rs b/src/app/transitions.rs deleted file mode 100644 index b176fa8..0000000 --- a/src/app/transitions.rs +++ /dev/null @@ -1,10 +0,0 @@ -#[allow(dead_code)] -/// Signals the outcome of a single user-input or system-driven state update. -pub enum AppTransition { - /// No state was mutated; the current frame can be reused. - Noop, - /// App state changed and a new frame should be rendered. - StateChanged, - /// The user requested the application to exit. - ExitRequested, -} From e1430734c6d74cc1971c0a6cc731d5805182506a Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Sat, 27 Jun 2026 14:55:44 -0300 Subject: [PATCH 02/10] refactor(app): trim AppHandle to run_until_done and remove AppMessage channel This commit resolves the Phase 12 follow-up by narrowing AppHandle to the single method main actually uses. The AppMessage control channel and its matching actor select branch are removed; the app loop runs on input events alone, and shutdown is driven by closing the input subscriber channel. This commit is part of the architecture's refactoring phase 13. Signed-off-by: lorenzoberts --- src/app/actor.rs | 108 ++++++++++++-------------------------------- src/app/handle.rs | 62 +++---------------------- src/app/messages.rs | 41 ----------------- src/app/mod.rs | 1 - 4 files changed, 36 insertions(+), 176 deletions(-) delete mode 100644 src/app/messages.rs diff --git a/src/app/actor.rs b/src/app/actor.rs index 2c8504e..519977e 100644 --- a/src/app/actor.rs +++ b/src/app/actor.rs @@ -1,6 +1,5 @@ use std::ops::ControlFlow; -use tokio::sync::mpsc; use tracing::{event, Level}; use crate::{ @@ -13,7 +12,6 @@ use crate::{ }, handle::AppHandle, loading::{terminal_error, TerminalLoadingIndicator}, - messages::AppMessage, screens::CurrentScreen, App, }, @@ -23,45 +21,40 @@ use crate::{ ui::handle::UiHandle, }; -pub const DEFAULT_APP_MSG_CHANNEL_SIZE: usize = 32; - /// Owns `App` state and drives the main application loop on a dedicated task. /// /// Constructed via [`AppActor::spawn`], which moves all owned resources into /// the actor and returns an [`AppHandle`] to the caller. +/// +/// The actor runs until the input event channel closes (the user requested +/// exit via the normal key binding) or an unrecoverable error occurs. pub struct AppActor { app: App, terminal_handle: TerminalHandle, ui_handle: UiHandle, input_handle: InputHandle, - event_rx: mpsc::Receiver, - msg_rx: mpsc::Receiver, + event_rx: tokio::sync::mpsc::Receiver, } impl AppActor { /// Moves all resources into a new `AppActor`, spawns it on the Tokio - /// runtime, and returns an [`AppHandle`] to control and wait on it. + /// runtime, and returns an [`AppHandle`] to wait on it. pub fn spawn( app: App, terminal_handle: TerminalHandle, ui_handle: UiHandle, input_handle: InputHandle, - event_rx: mpsc::Receiver, + event_rx: tokio::sync::mpsc::Receiver, ) -> AppHandle { - tracing::debug!( - channel_size = DEFAULT_APP_MSG_CHANNEL_SIZE, - "spawning app actor" - ); - let (msg_tx, msg_rx) = mpsc::channel(DEFAULT_APP_MSG_CHANNEL_SIZE); + tracing::debug!("spawning app actor"); let actor = Self { app, terminal_handle, ui_handle, input_handle, event_rx, - msg_rx, }; - AppHandle::new(tokio::spawn(actor.run()), msg_tx) + AppHandle::new(tokio::spawn(actor.run())) } /// Verifies required and optional external binaries. @@ -143,70 +136,27 @@ impl AppActor { .await .map_err(terminal_error)?; - enum Outcome { - Continue, - UpdateContext, - Stop, - } - - let outcome = tokio::select! { - input = self.event_rx.recv() => match input { - Some(event) => { - match on_input(&mut self.app, event, &self.terminal_handle, &mut loading) - .await? - { - ControlFlow::Continue(()) => Outcome::UpdateContext, - ControlFlow::Break(()) => Outcome::Stop, + match self.event_rx.recv().await { + Some(event) => { + match on_input(&mut self.app, event, &self.terminal_handle, &mut loading) + .await? + { + ControlFlow::Continue(()) => { + self.input_handle + .update_context(self.app.input_context()) + .await + .ok(); } + ControlFlow::Break(()) => break, } - None => { - tracing::info!("input channel closed; app actor stopping"); - Outcome::Stop - } - }, - msg = self.msg_rx.recv() => match msg { - Some(ref m) => { - tracing::debug!(message = m.name(), "app message received"); - match msg { - Some(AppMessage::Shutdown { reply_to }) => { - tracing::debug!("app actor shutting down"); - reply_to.send(()).ok(); - Outcome::Stop - } - Some(AppMessage::Initialize { reply_to }) => { - let result = self.initialize(); - if let Err(ref e) = result { - tracing::warn!(error = %e, "re-initialization failed"); - } - reply_to.send(result).ok(); - Outcome::Continue - } - Some(AppMessage::GetViewModel { reply_to }) => { - reply_to.send(self.app.present()).ok(); - Outcome::Continue - } - Some(AppMessage::GetInputContext { reply_to }) => { - reply_to.send(self.app.input_context()).ok(); - Outcome::Continue - } - None => Outcome::Continue, - } - } - None => Outcome::Continue, - }, - }; - - match outcome { - Outcome::Stop => break, - Outcome::UpdateContext => { - self.input_handle - .update_context(self.app.input_context()) - .await - .ok(); } - Outcome::Continue => {} + None => { + tracing::info!("input channel closed; app actor stopping"); + break; + } } } + tracing::info!("app actor stopped"); Ok(()) } @@ -270,7 +220,7 @@ mod tests { infrastructure::{ env::MockEnvTrait, file_system::MockFileSystemTrait, shell::MockShellTrait, }, - input::{handle::InputHandle, messages::InputMessage}, + input::{event::InputEvent, handle::InputHandle, messages::InputMessage}, lore::{application::handle::LoreApiHandle, domain::mailing_list::MailingList}, render::handle::RenderHandle, terminal::{ @@ -353,7 +303,7 @@ mod tests { } #[tokio::test(flavor = "multi_thread")] - async fn shutdown_message_stops_actor_and_returns_ok() { + async fn input_channel_close_stops_actor_and_returns_ok() { let mut session = MockTerminalSessionApi::new(); session .expect_draw() @@ -364,7 +314,7 @@ mod tests { let terminal_handle = TerminalActor::spawn(Box::new(session)); let ui_handle = UiActor::spawn(); - let (_event_tx, event_rx) = mpsc::channel::(1); + let (event_tx, event_rx) = mpsc::channel::(1); let (input_tx, _input_rx) = mpsc::channel::(1); let input_handle = InputHandle::new(input_tx); @@ -376,7 +326,9 @@ mod tests { event_rx, ); - handle.shutdown().await; + // Dropping the sender closes the channel; the actor stops after one + // render frame when event_rx.recv() returns None. + drop(event_tx); let result = handle.run_until_done().await; assert!(result.is_ok()); } diff --git a/src/app/handle.rs b/src/app/handle.rs index a0a0ae1..c4e9777 100644 --- a/src/app/handle.rs +++ b/src/app/handle.rs @@ -1,55 +1,17 @@ -use tokio::{ - sync::{mpsc, oneshot}, - task::JoinHandle, -}; +use tokio::task::JoinHandle; -use crate::{ - app::{errors::AppError, messages::AppMessage, view_model::AppViewModel}, - input::context::InputContext, -}; - -#[allow(dead_code)] /// Handle to the `AppActor` task. /// -/// Exposes typed async methods for controlling the actor and querying its -/// state. `run_until_done` consumes the handle and awaits the task's exit. +/// Returned by [`AppActor::spawn`] and consumed by [`AppHandle::run_until_done`]. +/// The actor runs until the input event channel closes (user exits) or an +/// unrecoverable error propagates from the run loop. pub struct AppHandle { join: JoinHandle>, - tx: mpsc::Sender, } -#[allow(dead_code)] impl AppHandle { - pub fn new(join: JoinHandle>, tx: mpsc::Sender) -> Self { - Self { join, tx } - } - - /// Requests the actor to run startup validation and returns the result. - pub async fn initialize(&self) -> Result<(), AppError> { - self.request(|reply_to| AppMessage::Initialize { reply_to }) - .await - .unwrap_or(Err(AppError::Input("actor channel closed".to_string()))) - } - - /// Requests the actor to stop its run loop and waits for acknowledgement. - pub async fn shutdown(&self) { - self.request(|reply_to| AppMessage::Shutdown { reply_to }) - .await - .ok(); - } - - /// Returns a snapshot of the current presentation model. - pub async fn get_view_model(&self) -> Option { - self.request(|reply_to| AppMessage::GetViewModel { reply_to }) - .await - .ok() - } - - /// Returns the current input context for the input mapper. - pub async fn get_input_context(&self) -> Option { - self.request(|reply_to| AppMessage::GetInputContext { reply_to }) - .await - .ok() + pub(crate) fn new(join: JoinHandle>) -> Self { + Self { join } } /// Blocks the caller until the actor task completes, propagating any error @@ -57,16 +19,4 @@ impl AppHandle { pub async fn run_until_done(self) -> color_eyre::Result<()> { self.join.await? } - - async fn request( - &self, - build_message: impl FnOnce(oneshot::Sender) -> AppMessage, - ) -> Result - where - T: Send + 'static, - { - let (reply_to, rx) = oneshot::channel(); - self.tx.send(build_message(reply_to)).await.ok(); - rx.await - } } diff --git a/src/app/messages.rs b/src/app/messages.rs deleted file mode 100644 index d9f883a..0000000 --- a/src/app/messages.rs +++ /dev/null @@ -1,41 +0,0 @@ -use tokio::sync::oneshot; - -use crate::{ - app::{errors::AppError, view_model::AppViewModel}, - input::context::InputContext, -}; - -/// Typed message protocol for the `AppActor`. -/// -/// External callers communicate with the actor exclusively through these -/// variants rather than touching `App` state directly. -pub enum AppMessage { - /// Request the actor to perform startup validation. - Initialize { - reply_to: oneshot::Sender>, - }, - - /// Request a snapshot of the current presentation model. - GetViewModel { - reply_to: oneshot::Sender, - }, - - /// Request the current input context for the input mapper. - GetInputContext { - reply_to: oneshot::Sender, - }, - - /// Request the actor to stop its run loop and exit cleanly. - Shutdown { reply_to: oneshot::Sender<()> }, -} - -impl AppMessage { - pub fn name(&self) -> &'static str { - match self { - AppMessage::Initialize { .. } => "Initialize", - AppMessage::GetViewModel { .. } => "GetViewModel", - AppMessage::GetInputContext { .. } => "GetInputContext", - AppMessage::Shutdown { .. } => "Shutdown", - } - } -} diff --git a/src/app/mod.rs b/src/app/mod.rs index 8b7b8e3..165bce6 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -4,7 +4,6 @@ pub(crate) mod flows; pub mod handle; pub mod input; pub(crate) mod loading; -pub mod messages; pub mod popup; pub mod screens; pub mod state; From bf65d62ad70d32d73e1f7f53004f3b4222f69ed3 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Sat, 27 Jun 2026 14:58:10 -0300 Subject: [PATCH 03/10] refactor(lore,render): remove LoreApi and Render handle methods with no callers This commit trims LoreApiHandle and RenderHandle to the methods exercised by production flows. Unused message variants, handle methods, and their test-only paths are removed, and stale dead_code suppressions on the actor protocol modules are dropped now that every remaining item has a caller. This commit is part of the architecture's refactoring phase 13. Signed-off-by: lorenzoberts --- src/lore/application/actor.rs | 16 -------- src/lore/application/handle.rs | 12 ------ src/lore/application/messages.rs | 10 ----- src/render/actor.rs | 65 +------------------------------- src/render/handle.rs | 26 ++----------- src/render/messages.rs | 14 +------ 6 files changed, 5 insertions(+), 138 deletions(-) diff --git a/src/lore/application/actor.rs b/src/lore/application/actor.rs index aa4c8ab..00a44ab 100644 --- a/src/lore/application/actor.rs +++ b/src/lore/application/actor.rs @@ -101,14 +101,6 @@ impl LoreApiActor { .and_then(|result| result); send_lore_reply(message_name, reply, result); } - LoreApiMessage::LoadBookmarks { reply } => { - tracing::debug!("loading bookmarked patchsets"); - let result = self - .with_core(|core| core.load_bookmarked_patchsets()) - .await - .and_then(|result| result); - send_lore_reply(message_name, reply, result); - } LoreApiMessage::SaveBookmarks { bookmarks, reply } => { tracing::debug!(count = bookmarks.len(), "saving bookmarked patchsets"); let result = self @@ -117,14 +109,6 @@ impl LoreApiActor { .and_then(|result| result); send_lore_reply(message_name, reply, result); } - LoreApiMessage::LoadReviewed { reply } => { - tracing::debug!("loading reviewed patchsets"); - let result = self - .with_core(|core| core.load_reviewed_patchsets()) - .await - .and_then(|result| result); - send_lore_reply(message_name, reply, result); - } LoreApiMessage::SaveReviewed { reviewed, reply } => { tracing::debug!(patchsets = reviewed.len(), "saving reviewed patchsets"); let result = self diff --git a/src/lore/application/handle.rs b/src/lore/application/handle.rs index 727cfc1..11f0d81 100644 --- a/src/lore/application/handle.rs +++ b/src/lore/application/handle.rs @@ -1,5 +1,3 @@ -#![allow(dead_code)] // Some protocol methods are reserved for future App flows. - use std::{ collections::{HashMap, HashSet}, path::PathBuf, @@ -73,21 +71,11 @@ impl LoreApiHandle { .await } - pub async fn load_bookmarks(&self) -> LoreApiResult> { - self.request_result(|reply| LoreApiMessage::LoadBookmarks { reply }) - .await - } - pub async fn save_bookmarks(&self, bookmarks: Vec) -> LoreApiResult<()> { self.request_result(|reply| LoreApiMessage::SaveBookmarks { bookmarks, reply }) .await } - pub async fn load_reviewed(&self) -> LoreApiResult>> { - self.request_result(|reply| LoreApiMessage::LoadReviewed { reply }) - .await - } - pub async fn save_reviewed( &self, reviewed: HashMap>, diff --git a/src/lore/application/messages.rs b/src/lore/application/messages.rs index 6926c43..4622583 100644 --- a/src/lore/application/messages.rs +++ b/src/lore/application/messages.rs @@ -1,5 +1,3 @@ -#![allow(dead_code)] // Follow-up commits wire the remaining message variants. - use std::{ collections::{HashMap, HashSet}, path::PathBuf, @@ -41,16 +39,10 @@ pub enum LoreApiMessage { cache_mode: CacheMode, reply: oneshot::Sender>, }, - LoadBookmarks { - reply: oneshot::Sender>>, - }, SaveBookmarks { bookmarks: Vec, reply: oneshot::Sender>, }, - LoadReviewed { - reply: oneshot::Sender>>>, - }, SaveReviewed { reviewed: HashMap>, reply: oneshot::Sender>, @@ -77,9 +69,7 @@ impl LoreApiMessage { LoreApiMessage::FetchAvailableLists { .. } => "FetchAvailableLists", LoreApiMessage::FetchFeedPage { .. } => "FetchFeedPage", LoreApiMessage::FetchPatchsetDetails { .. } => "FetchPatchsetDetails", - LoreApiMessage::LoadBookmarks { .. } => "LoadBookmarks", LoreApiMessage::SaveBookmarks { .. } => "SaveBookmarks", - LoreApiMessage::LoadReviewed { .. } => "LoadReviewed", LoreApiMessage::SaveReviewed { .. } => "SaveReviewed", LoreApiMessage::GetGitSignature { .. } => "GetGitSignature", LoreApiMessage::PrepareReplyCommands { .. } => "PrepareReplyCommands", diff --git a/src/render/actor.rs b/src/render/actor.rs index 3b9cb9f..a79b77e 100644 --- a/src/render/actor.rs +++ b/src/render/actor.rs @@ -6,7 +6,7 @@ use tokio::{ use crate::render::{ handle::RenderHandle, messages::{RenderMessage, RenderResult}, - RenderError, RenderPatchsetRequest, RenderServiceApi, + RenderError, RenderServiceApi, }; pub const DEFAULT_RENDER_CHANNEL_SIZE: usize = 32; @@ -60,35 +60,6 @@ impl RenderActor { .and_then(|result| result); send_render_reply(message_name, reply, result); } - RenderMessage::RenderSinglePatch { - raw_patch, - patch_renderer, - cover_renderer, - reply, - } => { - tracing::debug!( - patch_renderer = %patch_renderer, - cover_renderer = %cover_renderer, - "rendering single patch preview" - ); - let result = self - .with_core(move |core| { - let request = RenderPatchsetRequest::new( - vec![raw_patch], - patch_renderer, - cover_renderer, - ); - core.render_patchset_preview(request) - }) - .await - .and_then(|result| result) - .and_then(|preview| { - preview.entries.into_iter().next().ok_or_else(|| { - RenderError::Failed("render returned no preview entries".to_string()) - }) - }); - send_render_reply(message_name, reply, result); - } } } @@ -170,38 +141,4 @@ mod tests { assert!(rendered.entries[0].rendered_text.contains("+line")); assert!(rendered.entries[1].rendered_text.contains("-line")); } - - #[tokio::test] - async fn render_single_patch_returns_first_preview_entry() { - let handle = spawn_test_actor(); - - let rendered = handle - .render_single_patch( - "subject\n\nbody\n---\n+line\n".to_string(), - PatchRenderer::Default, - CoverRenderer::Default, - ) - .await - .expect("default renderers should not spawn"); - - assert!(rendered.rendered_text.contains("+line")); - } - - #[tokio::test] - async fn render_actor_handles_sequential_requests() { - let handle = spawn_test_actor(); - - for content in ["+first", "+second"] { - let rendered = handle - .render_single_patch( - format!("subject\n\nbody\n---\n{content}\n"), - PatchRenderer::Default, - CoverRenderer::Default, - ) - .await - .expect("default renderers should not spawn"); - - assert!(rendered.rendered_text.contains(content)); - } - } } diff --git a/src/render/handle.rs b/src/render/handle.rs index 46038c7..9e6d252 100644 --- a/src/render/handle.rs +++ b/src/render/handle.rs @@ -1,13 +1,8 @@ -#![allow(dead_code)] // Some protocol methods are reserved for future App flows. - use tokio::sync::{mpsc, oneshot}; -use crate::{ - render::{ - messages::{RenderMessage, RenderResult}, - RenderError, RenderPatchsetRequest, RenderedPatchPreview, RenderedPatchsetPreview, - }, - render_prefs::{CoverRenderer, PatchRenderer}, +use crate::render::{ + messages::{RenderMessage, RenderResult}, + RenderError, RenderPatchsetRequest, RenderedPatchsetPreview, }; #[derive(Clone)] @@ -28,21 +23,6 @@ impl RenderHandle { .await } - pub async fn render_single_patch( - &self, - raw_patch: String, - patch_renderer: PatchRenderer, - cover_renderer: CoverRenderer, - ) -> RenderResult { - self.request_result(|reply| RenderMessage::RenderSinglePatch { - raw_patch, - patch_renderer, - cover_renderer, - reply, - }) - .await - } - async fn request_result( &self, build_message: impl FnOnce(oneshot::Sender>) -> RenderMessage, diff --git a/src/render/messages.rs b/src/render/messages.rs index c06a0ee..1b4dc22 100644 --- a/src/render/messages.rs +++ b/src/render/messages.rs @@ -1,11 +1,6 @@ -#![allow(dead_code)] // Follow-up phases may wire additional render message variants. - use tokio::sync::oneshot; -use crate::{ - render::{RenderError, RenderPatchsetRequest, RenderedPatchPreview, RenderedPatchsetPreview}, - render_prefs::{CoverRenderer, PatchRenderer}, -}; +use crate::render::{RenderError, RenderPatchsetRequest, RenderedPatchsetPreview}; pub type RenderResult = Result; @@ -14,19 +9,12 @@ pub enum RenderMessage { request: RenderPatchsetRequest, reply: oneshot::Sender>, }, - RenderSinglePatch { - raw_patch: String, - patch_renderer: PatchRenderer, - cover_renderer: CoverRenderer, - reply: oneshot::Sender>, - }, } impl RenderMessage { pub fn name(&self) -> &'static str { match self { RenderMessage::RenderPatchsetPreview { .. } => "RenderPatchsetPreview", - RenderMessage::RenderSinglePatch { .. } => "RenderSinglePatch", } } } From f66ac8fd70bbe8b05cc83c1edb3121c25a615eeb Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Sat, 27 Jun 2026 15:10:04 -0300 Subject: [PATCH 04/10] refactor(app): keep ratatui types out of PatchsetDetailsState This commit removes a ratatui dependency from application state by storing rendered preview content as raw strings and deferring Text conversion to the view-model projection layer. from_rendered_preview becomes infallible, keeping presentation types on the UI side of the actor boundary. This commit is part of the architecture's refactoring phase 13. Signed-off-by: lorenzoberts --- src/app/mod.rs | 2 +- src/app/screens/details_actions.rs | 27 ++++++++++++--------------- src/app/view_model.rs | 7 ++++++- 3 files changed, 19 insertions(+), 17 deletions(-) diff --git a/src/app/mod.rs b/src/app/mod.rs index 165bce6..54806bd 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -235,7 +235,7 @@ impl App { rendered_preview, is_patchset_bookmarked, self.state.navigation.current_screen.clone(), - )?); + )); Ok(B4Result::PatchFound) } diff --git a/src/app/screens/details_actions.rs b/src/app/screens/details_actions.rs index e9be519..7218225 100644 --- a/src/app/screens/details_actions.rs +++ b/src/app/screens/details_actions.rs @@ -1,6 +1,3 @@ -use ansi_to_tui::IntoText; -use ratatui::text::Text; - use std::collections::{HashMap, HashSet}; use std::path::Path; @@ -24,14 +21,14 @@ pub struct PatchsetDetailsState { pub representative_patch: Patch, /// Raw patches as plain text files pub raw_patches: Vec, - /// Patches in the format to be displayed as preview - pub patches_preview: Vec>, + /// ANSI-rendered text for each patch entry, converted to ratatui `Text` by + /// the ViewModel projection rather than stored as a UI type. + pub patches_preview: Vec, /// Indicates if patchset has a cover letter pub has_cover_letter: bool, /// Which patches to reply pub patches_to_reply: Vec, - /// Path to applicable .mbx of patchset - #[allow(dead_code)] + /// Path to the .mbx file used by `git am` when applying the patchset. pub patchset_path: String, pub preview_index: usize, pub preview_scroll_offset: usize, @@ -65,8 +62,8 @@ impl PatchsetDetailsState { rendered_preview: RenderedPatchsetPreview, is_patchset_bookmarked: bool, last_screen: CurrentScreen, - ) -> color_eyre::Result { - let mut patches_preview: Vec = Vec::new(); + ) -> Self { + let mut patches_preview: Vec = Vec::new(); let mut reviewed_by: Vec> = Vec::new(); let mut tested_by: Vec> = Vec::new(); let mut acked_by: Vec> = Vec::new(); @@ -79,13 +76,13 @@ impl PatchsetDetailsState { reviewed_by.push(tag_summary.reviewed_by.clone()); tested_by.push(tag_summary.tested_by.clone()); acked_by.push(tag_summary.acked_by.clone()); - patches_preview.push(entry.rendered_text.as_str().into_text()?); + patches_preview.push(entry.rendered_text.clone()); } let has_cover_letter = representative_patch.number_in_series() == 0; let patches_to_reply = vec![false; details.raw_patches.len()]; - Ok(Self { + Self { representative_patch, raw_patches: details.raw_patches, patchset_path: details.patchset_path, @@ -105,7 +102,7 @@ impl PatchsetDetailsState { tested_by, acked_by, last_screen, - }) + } } pub fn preview_next_patch(&mut self) { @@ -127,7 +124,7 @@ impl PatchsetDetailsState { /// Scroll `n` lines down pub fn preview_scroll_down(&mut self, n: usize) { // TODO: Support for renderers (only considers base preview string) - let number_of_lines = self.patches_preview[self.preview_index].height(); + let number_of_lines = self.patches_preview[self.preview_index].lines().count(); if (self.preview_scroll_offset + n) <= number_of_lines { self.preview_scroll_offset += n; } @@ -141,8 +138,8 @@ impl PatchsetDetailsState { /// Scroll to the last line pub fn go_to_last_line(&mut self) { // TODO: Support for renderers (only considers base preview string) - let number_of_lines = self.patches_preview[self.preview_index].height(); - self.preview_scroll_offset = number_of_lines - LAST_LINE_PADDING; + let number_of_lines = self.patches_preview[self.preview_index].lines().count(); + self.preview_scroll_offset = number_of_lines.saturating_sub(LAST_LINE_PADDING); } /// Scroll to first line diff --git a/src/app/view_model.rs b/src/app/view_model.rs index 78fc84e..c130f0a 100644 --- a/src/app/view_model.rs +++ b/src/app/view_model.rs @@ -7,6 +7,7 @@ //! about the presentation before `UiCore` translates them into paint-ready //! `UiScene` nodes. +use ansi_to_tui::IntoText; use ratatui::text::Text; use super::{ @@ -359,7 +360,11 @@ fn project_details(state: &AppState) -> PatchsetDetailsViewModel { acked_by: details.acked_by[i].len(), }, staged_to_reply, - preview_entries: details.patches_preview.clone(), + preview_entries: details + .patches_preview + .iter() + .map(|s| s.as_str().into_text().unwrap_or_default()) + .collect(), preview_index: i, preview_scroll_offset: details.preview_scroll_offset, preview_pan: details.preview_pan, From 2b28022615b0bcdc820293262bb413f0b75706eb Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Sat, 27 Jun 2026 15:15:19 -0300 Subject: [PATCH 05/10] refactor(app,config): replace bare unwrap with documented invariant expect This commit replaces silent unwrap calls in production app and config paths with expect messages that document the program invariants each site depends on. Success-path is_ok plus unwrap pairs in screen flows are collapsed into straightforward Result handling. This commit is part of the architecture's refactoring phase 13. Signed-off-by: lorenzoberts --- src/app/flows/bookmarked.rs | 11 +++++----- src/app/flows/details_actions.rs | 7 ++++++- src/app/flows/latest.rs | 14 ++++++------- src/app/flows/mail_list.rs | 2 +- src/app/mod.rs | 35 ++++++++++++++++++++++---------- src/app/screens/bookmarked.rs | 2 +- src/app/screens/latest.rs | 5 ++++- src/app/updates.rs | 7 ++++++- src/config/repository.rs | 3 ++- 9 files changed, 56 insertions(+), 30 deletions(-) diff --git a/src/app/flows/bookmarked.rs b/src/app/flows/bookmarked.rs index cfc2aef..b91cbd2 100644 --- a/src/app/flows/bookmarked.rs +++ b/src/app/flows/bookmarked.rs @@ -36,12 +36,11 @@ pub async fn handle_bookmarked_patchsets( loading.start("Loading patchset".to_string()); let result = app.open_patchset_details().await; loading.stop()?; - if result.is_ok() { - // If a patchset has been bookmarked UI, this means that - // b4 was successful in fetching it, so it shouldn't be - // necessary to handle this, but we can't assume that a - // patchset in this list was bookmarked through the UI - match result.unwrap() { + // If a patchset has been bookmarked via the UI, b4 was already + // successful for it, but patchsets may also arrive here from + // other sources where the fetch could fail. + if let Ok(b4_result) = result { + match b4_result { B4Result::PatchFound => { app.set_current_screen(CurrentScreen::PatchsetDetails); } diff --git a/src/app/flows/details_actions.rs b/src/app/flows/details_actions.rs index a022a37..28b18ae 100644 --- a/src/app/flows/details_actions.rs +++ b/src/app/flows/details_actions.rs @@ -16,7 +16,12 @@ pub async fn handle_patchset_details( input: InputEvent, terminal_handle: &TerminalHandle, ) -> color_eyre::Result<()> { - let patchset_details_and_actions = app.state.lore.details.as_mut().unwrap(); + let patchset_details_and_actions = app + .state + .lore + .details + .as_mut() + .expect("invariant: details must be loaded before handling patchset details input"); match input { InputEvent::OpenHelp => { diff --git a/src/app/flows/latest.rs b/src/app/flows/latest.rs index 99ffbbb..2193243 100644 --- a/src/app/flows/latest.rs +++ b/src/app/flows/latest.rs @@ -24,7 +24,7 @@ pub async fn handle_latest_patchsets( .lore .latest_patchsets .as_mut() - .unwrap() + .expect("invariant: latest_patchsets must be initialised on LatestPatchsets screen") .select_below_patchset(); } InputEvent::NavigateUp => { @@ -32,7 +32,7 @@ pub async fn handle_latest_patchsets( .lore .latest_patchsets .as_mut() - .unwrap() + .expect("invariant: latest_patchsets must be initialised on LatestPatchsets screen") .select_above_patchset(); } InputEvent::NextPage => { @@ -41,7 +41,7 @@ pub async fn handle_latest_patchsets( .lore .latest_patchsets .as_ref() - .unwrap() + .expect("invariant: latest_patchsets must be initialised on LatestPatchsets screen") .target_list() .to_string(); debug!(list = list_name, "fetching next page of patchsets"); @@ -50,7 +50,7 @@ pub async fn handle_latest_patchsets( .lore .latest_patchsets .as_mut() - .unwrap() + .expect("invariant: latest_patchsets must be initialised on LatestPatchsets screen") .increment_page(); let result = app.fetch_latest_current_page().await; loading.stop()?; @@ -61,7 +61,7 @@ pub async fn handle_latest_patchsets( .lore .latest_patchsets .as_mut() - .unwrap() + .expect("invariant: latest_patchsets must be initialised on LatestPatchsets screen") .decrement_page(); // Reload from cache (no network call since LoreAPI caches all pages) app.fetch_latest_current_page().await?; @@ -71,8 +71,8 @@ pub async fn handle_latest_patchsets( loading.start("Loading patchset".to_string()); let result = app.open_patchset_details().await; loading.stop()?; - if result.is_ok() { - match result.unwrap() { + if let Ok(b4_result) = result { + match b4_result { B4Result::PatchFound => { app.set_current_screen(CurrentScreen::PatchsetDetails); } diff --git a/src/app/flows/mail_list.rs b/src/app/flows/mail_list.rs index aa0a743..bea7031 100644 --- a/src/app/flows/mail_list.rs +++ b/src/app/flows/mail_list.rs @@ -30,7 +30,7 @@ pub async fn handle_mailing_list_selection( .lore .latest_patchsets .as_ref() - .unwrap() + .expect("invariant: init_latest_patchsets was just called") .target_list() .to_string(); diff --git a/src/app/mod.rs b/src/app/mod.rs index 54806bd..b744a02 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -191,7 +191,7 @@ impl App { .lore .latest_patchsets .as_ref() - .unwrap() + .expect("invariant: latest_patchsets must be initialised before opening details") .get_selected_patchset(); if !self .state @@ -258,7 +258,12 @@ impl App { } async fn sync_patchset_bookmark(&mut self) -> color_eyre::Result<()> { - let details = self.state.lore.details.as_ref().unwrap(); + let details = self + .state + .lore + .details + .as_ref() + .expect("invariant: details must be loaded before consolidating patchset actions"); let representative_patch = &details.representative_patch; let patchset_actions = &details.patchset_actions; @@ -289,7 +294,12 @@ impl App { } async fn execute_reviewed_reply(&mut self) -> color_eyre::Result<()> { - let details = self.state.lore.details.as_ref().unwrap(); + let details = self + .state + .lore + .details + .as_ref() + .expect("invariant: details must be loaded before executing reviewed reply"); let representative_patch = details.representative_patch.clone(); let patchset_actions = &details.patchset_actions; let raw_patches = details.raw_patches.clone(); @@ -372,7 +382,7 @@ impl App { .lore .details .as_mut() - .unwrap() + .expect("invariant: details must be loaded before resetting reply action") .reset_reply_with_reviewed_by_action(); } Ok(()) @@ -384,15 +394,18 @@ impl App { .lore .details .as_ref() - .unwrap() + .expect("invariant: details must be loaded before executing apply patchset") .patchset_actions .get(&PatchsetAction::Apply) { - let popup = match self.state.lore.details.as_ref().unwrap().apply_patchset( - &*self.services.fs, - &*self.services.shell, - &self.state.config, - ) { + let popup = match self + .state + .lore + .details + .as_ref() + .expect("invariant: details must be loaded before applying patchset") + .apply_patchset(&*self.services.fs, &*self.services.shell, &self.state.config) + { Ok(msg) => popup::AppPopup::info("Patchset Apply Success", msg), Err(msg) => popup::AppPopup::info("Patchset Apply Fail", msg), }; @@ -403,7 +416,7 @@ impl App { .lore .details .as_mut() - .unwrap() + .expect("invariant: details must be loaded before toggling apply action") .toggle_apply_action(); } } diff --git a/src/app/screens/bookmarked.rs b/src/app/screens/bookmarked.rs index 88a2531..2a08ff4 100644 --- a/src/app/screens/bookmarked.rs +++ b/src/app/screens/bookmarked.rs @@ -20,7 +20,7 @@ impl BookmarkedPatchsetsState { pub fn get_selected_patchset(&self) -> Patch { self.bookmarked_patchsets .get(self.patchset_index) - .unwrap() + .expect("invariant: patchset_index must be within bookmarked_patchsets bounds") .clone() } diff --git a/src/app/screens/latest.rs b/src/app/screens/latest.rs index 53c003d..589dc23 100644 --- a/src/app/screens/latest.rs +++ b/src/app/screens/latest.rs @@ -94,7 +94,10 @@ impl LatestPatchsetsState { } pub fn get_selected_patchset(&self) -> Patch { - self.current_page.get(self.patchset_index).unwrap().clone() + self.current_page + .get(self.patchset_index) + .expect("invariant: patchset_index must be within current_page bounds") + .clone() } pub fn get_current_patch_feed_page(&self) -> Option> { diff --git a/src/app/updates.rs b/src/app/updates.rs index 2084b08..1e378bf 100644 --- a/src/app/updates.rs +++ b/src/app/updates.rs @@ -22,7 +22,12 @@ impl App { } } CurrentScreen::LatestPatchsets => { - let patchsets_state = self.state.lore.latest_patchsets.as_ref().unwrap(); + let patchsets_state = self + .state + .lore + .latest_patchsets + .as_ref() + .expect("invariant: latest_patchsets must be initialised on LatestPatchsets screen"); if patchsets_state.processed_patchsets_count() == 0 { let target_list = patchsets_state.target_list().to_string(); diff --git a/src/config/repository.rs b/src/config/repository.rs index 2c7f587..9f81cf7 100644 --- a/src/config/repository.rs +++ b/src/config/repository.rs @@ -15,7 +15,8 @@ pub fn resolve_config_path(env: &dyn EnvTrait) -> String { env.var("PATCH_HUB_CONFIG_PATH").unwrap_or_else(|_| { format!( "{}/{}", - env.var("HOME").unwrap(), + env.var("HOME") + .expect("invariant: HOME environment variable must be set"), DEFAULT_CONFIG_PATH_SUFFIX ) }) From 126b4caa72742d30171c8d4e69f90667115bd7f4 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Sat, 27 Jun 2026 15:19:59 -0300 Subject: [PATCH 06/10] refactor(app): add tracing to cross-actor application flows This commit adds structured tracing at LoreAPI, Render, and Config boundaries inside the main application flows. Patchset loading, list refresh, bookmark persistence, and configuration saves are now observable in the log stream without changing behavior. This commit is part of the architecture's refactoring phase 13. Signed-off-by: lorenzoberts --- src/app/mod.rs | 49 +++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 43 insertions(+), 6 deletions(-) diff --git a/src/app/mod.rs b/src/app/mod.rs index b744a02..29a442d 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -11,7 +11,7 @@ pub mod updates; pub mod view_model; use color_eyre::eyre::{bail, eyre}; -use tracing::{event, Level}; +use tracing::{debug, event, info, warn, Level}; use std::path::PathBuf; @@ -155,9 +155,17 @@ impl App { let lore_api = &self.services.lore_api; let latest_patchsets = &mut self.state.lore.latest_patchsets; if let Some(patchsets) = latest_patchsets.as_mut() { - patchsets + let list = patchsets.target_list().to_string(); + let page = patchsets.page_number(); + debug!(list, page, "fetching latest patchsets page"); + let result = patchsets .fetch_current_page(lore_api, CacheMode::UseCache) - .await + .await; + match &result { + Ok(()) => debug!(list, page, "latest patchsets page fetched"), + Err(e) => warn!(list, page, error = %e, "failed to fetch latest patchsets page"), + } + result } else { Ok(()) } @@ -165,11 +173,18 @@ impl App { /// Refreshes available mailing lists and updates [`LoreUiState::mailing_list_selection`]. pub async fn refresh_mailing_lists(&mut self) -> color_eyre::Result<()> { - self.state + debug!("refreshing mailing lists"); + let result = self + .state .lore .mailing_list_selection .refresh_available_mailing_lists(&self.services.lore_api, CacheMode::Refresh) - .await + .await; + match &result { + Ok(()) => debug!("mailing lists refreshed"), + Err(e) => warn!(error = %e, "failed to refresh mailing lists"), + } + result } /// Loads patchset details into [`LoreUiState::details`]. @@ -206,6 +221,9 @@ impl App { screen => bail!(format!("Invalid screen passed as argument {screen:?}")), }; + let msg_id = &representative_patch.message_id().href; + debug!(msg_id, "fetching patchset details from LoreAPI"); + let details = match self .services .lore_api @@ -213,10 +231,14 @@ impl App { .await { Ok(d) => d, - Err(LoreError::PatchNotFound(err)) => return Ok(B4Result::PatchNotFound(err)), + Err(LoreError::PatchNotFound(err)) => { + warn!(msg_id, reason = err, "patchset not found"); + return Ok(B4Result::PatchNotFound(err)); + } Err(e) => bail!("{e:#?}"), }; + debug!(msg_id, patches = details.raw_patches.len(), "rendering patchset preview"); let render_request = RenderPatchsetRequest::new( details.raw_patches.clone(), *self.state.config.patch_renderer(), @@ -229,6 +251,7 @@ impl App { .await .map_err(|e| eyre!("{e}"))?; + debug!(msg_id, "patchset details loaded"); self.state.lore.details = Some(PatchsetDetailsState::from_rendered_preview( representative_patch, details, @@ -251,9 +274,11 @@ impl App { /// /// Panics if [`LoreUiState::details`] is `None`. pub async fn consolidate_patchset_actions(&mut self) -> color_eyre::Result<()> { + debug!("consolidating patchset actions"); self.sync_patchset_bookmark().await?; self.execute_reviewed_reply().await?; self.execute_apply_patchset(); + debug!("patchset actions consolidated"); Ok(()) } @@ -266,13 +291,16 @@ impl App { .expect("invariant: details must be loaded before consolidating patchset actions"); let representative_patch = &details.representative_patch; let patchset_actions = &details.patchset_actions; + let msg_id = &representative_patch.message_id().href; if let Some(true) = patchset_actions.get(&PatchsetAction::Bookmark) { + debug!(msg_id, "bookmarking patchset"); self.state .user_state .bookmarked_patchsets .bookmark_selected_patch(representative_patch); } else { + debug!(msg_id, "unbookmarking patchset"); self.state .user_state .bookmarked_patchsets @@ -290,6 +318,7 @@ impl App { ) .await .map_err(|e| eyre!("{e:#?}"))?; + debug!(msg_id, "bookmark state persisted"); Ok(()) } @@ -306,6 +335,7 @@ impl App { let patches_to_reply = details.patches_to_reply.clone(); if let Some(true) = patchset_actions.get(&PatchsetAction::ReplyWithReviewedBy) { + debug!(msg_id = representative_patch.message_id().href, "executing reviewed-by reply"); let mut successful_indexes = self .state .user_state @@ -378,6 +408,10 @@ impl App { .await .map_err(|e| eyre!("{e:#?}"))?; + info!( + msg_id = representative_patch.message_id().href, + "reviewed-by reply sent and state persisted" + ); self.state .lore .details @@ -398,6 +432,7 @@ impl App { .patchset_actions .get(&PatchsetAction::Apply) { + debug!("applying patchset via git-am"); let popup = match self .state .lore @@ -433,10 +468,12 @@ impl App { /// Applies edited values from [`ConfigUiState::edit_config`] into [`AppState::config`]. pub fn consolidate_edit_config(&mut self) -> color_eyre::Result<()> { if let Some(edit_config) = &self.state.config_state.edit_config { + debug!("validating and applying config update"); let draft = edit_config.to_update_draft(); let validated = self.services.config.validate_update(draft)?; let snapshot = self.services.config.apply_update(validated)?; self.state.config = snapshot; + info!("configuration updated and persisted"); } Ok(()) } From 4f6512d5c2eb39a39d8db26ea8b3e635d8790c40 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Sat, 27 Jun 2026 15:23:38 -0300 Subject: [PATCH 07/10] refactor(lore,render): add explicit Shutdown messages and ordered teardown This commit replaces implicit actor teardown on channel drop with explicit Shutdown messages on LoreApiActor and RenderActor. main retains handles to those actors after AppActor finishes and shuts them down in a documented order before UI and terminal teardown, so long-running actor tasks stop predictably. This commit is part of the architecture's refactoring phase 13. Signed-off-by: lorenzoberts --- src/lore/application/actor.rs | 20 ++++++++++++++++++-- src/lore/application/handle.rs | 9 +++++++++ src/lore/application/messages.rs | 2 ++ src/main.rs | 13 +++++++++++-- src/render/actor.rs | 13 +++++++++++-- src/render/handle.rs | 9 +++++++++ src/render/messages.rs | 2 ++ 7 files changed, 62 insertions(+), 6 deletions(-) diff --git a/src/lore/application/actor.rs b/src/lore/application/actor.rs index 00a44ab..a6bfd73 100644 --- a/src/lore/application/actor.rs +++ b/src/lore/application/actor.rs @@ -1,3 +1,5 @@ +use std::ops::ControlFlow; + use tokio::{ sync::{mpsc, oneshot}, task, @@ -35,12 +37,14 @@ impl LoreApiActor { pub async fn run(mut self) { tracing::info!("lore api actor started"); while let Some(message) = self.rx.recv().await { - self.handle_message(message).await; + if let ControlFlow::Break(()) = self.handle_message(message).await { + break; + } } tracing::info!("lore api actor stopped"); } - async fn handle_message(&mut self, message: LoreApiMessage) { + async fn handle_message(&mut self, message: LoreApiMessage) -> ControlFlow<()> { let message_name = message.name(); tracing::debug!(message = message_name, "lore api request received"); @@ -52,6 +56,7 @@ impl LoreApiActor { .await .and_then(|result| result); send_lore_reply(message_name, reply, result); + ControlFlow::Continue(()) } LoreApiMessage::FetchAvailableLists { cache_mode, reply } => { tracing::debug!(?cache_mode, "fetching available mailing lists"); @@ -60,6 +65,7 @@ impl LoreApiActor { .await .and_then(|result| result); send_lore_reply(message_name, reply, result); + ControlFlow::Continue(()) } LoreApiMessage::FetchFeedPage { target_list, @@ -82,6 +88,7 @@ impl LoreApiActor { .await .and_then(|result| result); send_lore_reply(message_name, reply, result); + ControlFlow::Continue(()) } LoreApiMessage::FetchPatchsetDetails { representative_patch, @@ -100,6 +107,7 @@ impl LoreApiActor { .await .and_then(|result| result); send_lore_reply(message_name, reply, result); + ControlFlow::Continue(()) } LoreApiMessage::SaveBookmarks { bookmarks, reply } => { tracing::debug!(count = bookmarks.len(), "saving bookmarked patchsets"); @@ -108,6 +116,7 @@ impl LoreApiActor { .await .and_then(|result| result); send_lore_reply(message_name, reply, result); + ControlFlow::Continue(()) } LoreApiMessage::SaveReviewed { reviewed, reply } => { tracing::debug!(patchsets = reviewed.len(), "saving reviewed patchsets"); @@ -116,6 +125,7 @@ impl LoreApiActor { .await .and_then(|result| result); send_lore_reply(message_name, reply, result); + ControlFlow::Continue(()) } LoreApiMessage::GetGitSignature { git_repo_path, @@ -126,6 +136,7 @@ impl LoreApiActor { .with_core(move |core| core.get_git_signature(&git_repo_path)) .await; send_lore_reply(message_name, reply, result); + ControlFlow::Continue(()) } LoreApiMessage::PrepareReplyCommands { tmp_dir, @@ -156,6 +167,11 @@ impl LoreApiActor { .await .and_then(|result| result); send_lore_reply(message_name, reply, result); + ControlFlow::Continue(()) + } + LoreApiMessage::Shutdown => { + tracing::debug!("lore api actor shutting down"); + ControlFlow::Break(()) } } } diff --git a/src/lore/application/handle.rs b/src/lore/application/handle.rs index 11f0d81..5b739e9 100644 --- a/src/lore/application/handle.rs +++ b/src/lore/application/handle.rs @@ -95,6 +95,15 @@ impl LoreApiHandle { .await } + /// Signals the actor to stop processing messages and exit its run loop. + /// + /// Callers should invoke this after the last request that uses this handle has + /// completed. Dropping all clones of the handle also stops the actor, but + /// calling `shutdown` makes the intent explicit and allows ordered teardown. + pub async fn shutdown(&self) { + self.tx.send(LoreApiMessage::Shutdown).await.ok(); + } + pub async fn prepare_reply_commands( &self, tmp_dir: PathBuf, diff --git a/src/lore/application/messages.rs b/src/lore/application/messages.rs index 4622583..81ba640 100644 --- a/src/lore/application/messages.rs +++ b/src/lore/application/messages.rs @@ -60,6 +60,7 @@ pub enum LoreApiMessage { git_send_email_options: String, reply: oneshot::Sender>>, }, + Shutdown, } impl LoreApiMessage { @@ -73,6 +74,7 @@ impl LoreApiMessage { LoreApiMessage::SaveReviewed { .. } => "SaveReviewed", LoreApiMessage::GetGitSignature { .. } => "GetGitSignature", LoreApiMessage::PrepareReplyCommands { .. } => "PrepareReplyCommands", + LoreApiMessage::Shutdown => "Shutdown", } } } diff --git a/src/main.rs b/src/main.rs index 7fd36ff..f06a3e3 100644 --- a/src/main.rs +++ b/src/main.rs @@ -115,8 +115,8 @@ async fn main() -> color_eyre::Result<()> { Box::new(OsFileSystem), Box::new(OsShell), Box::new(env), - lore_api, - render, + lore_api.clone(), + render.clone(), )?; let (app_input_tx, app_input_rx) = mpsc::channel::(64); let input_handle = InputActor::spawn(terminal_handle.clone(), app.input_context()); @@ -125,6 +125,13 @@ async fn main() -> color_eyre::Result<()> { .await .map_err(|e| eyre!("{e}"))?; + // Shutdown ordering: + // 1. AppActor — exits when the user quits (input channel closes) + // 2. LoreApiActor — no further requests once App is gone + // 3. RenderActor — no further requests once App is gone + // 4. UiActor — no further scene builds once App is gone + // 5. TerminalActor — restores the terminal last so the screen stays usable + // during the steps above AppActor::spawn( app, terminal_handle.clone(), @@ -134,6 +141,8 @@ async fn main() -> color_eyre::Result<()> { ) .run_until_done() .await?; + lore_api.shutdown().await; + render.shutdown().await; ui_handle.shutdown().await; terminal_handle .shutdown() diff --git a/src/render/actor.rs b/src/render/actor.rs index a79b77e..cbadf63 100644 --- a/src/render/actor.rs +++ b/src/render/actor.rs @@ -1,3 +1,5 @@ +use std::ops::ControlFlow; + use tokio::{ sync::{mpsc, oneshot}, task, @@ -37,12 +39,14 @@ impl RenderActor { pub async fn run(mut self) { tracing::info!("render actor started"); while let Some(message) = self.rx.recv().await { - self.handle_message(message).await; + if let ControlFlow::Break(()) = self.handle_message(message).await { + break; + } } tracing::info!("render actor stopped"); } - async fn handle_message(&mut self, message: RenderMessage) { + async fn handle_message(&mut self, message: RenderMessage) -> ControlFlow<()> { let message_name = message.name(); tracing::debug!(message = message_name, "render request received"); @@ -59,6 +63,11 @@ impl RenderActor { .await .and_then(|result| result); send_render_reply(message_name, reply, result); + ControlFlow::Continue(()) + } + RenderMessage::Shutdown => { + tracing::debug!("render actor shutting down"); + ControlFlow::Break(()) } } } diff --git a/src/render/handle.rs b/src/render/handle.rs index 9e6d252..8a96414 100644 --- a/src/render/handle.rs +++ b/src/render/handle.rs @@ -15,6 +15,15 @@ impl RenderHandle { Self { tx } } + /// Signals the actor to stop processing messages and exit its run loop. + /// + /// Callers should invoke this after the last request that uses this handle has + /// completed. Dropping all clones of the handle also stops the actor, but + /// calling `shutdown` makes the intent explicit and allows ordered teardown. + pub async fn shutdown(&self) { + self.tx.send(RenderMessage::Shutdown).await.ok(); + } + pub async fn render_patchset_preview( &self, request: RenderPatchsetRequest, diff --git a/src/render/messages.rs b/src/render/messages.rs index 1b4dc22..c231fab 100644 --- a/src/render/messages.rs +++ b/src/render/messages.rs @@ -9,12 +9,14 @@ pub enum RenderMessage { request: RenderPatchsetRequest, reply: oneshot::Sender>, }, + Shutdown, } impl RenderMessage { pub fn name(&self) -> &'static str { match self { RenderMessage::RenderPatchsetPreview { .. } => "RenderPatchsetPreview", + RenderMessage::Shutdown => "Shutdown", } } } From 923010b263c6a2db641bd100bf80a974ff9a4b66 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Sat, 27 Jun 2026 15:28:44 -0300 Subject: [PATCH 08/10] test(app,input): add multi-actor integration lifecycle tests This commit adds integration tests that exercise real LoreApiActor and RenderActor instances through AppActor startup, and verifies that a terminal poll error propagates through InputActor to close the app subscriber channel. These paths were previously covered only with stub channels or left untested. This commit is part of the architecture's refactoring phase 13. Signed-off-by: lorenzoberts --- src/app/actor.rs | 107 +++++++++++++++++++++++++++++++++++++++++++-- src/input/actor.rs | 23 +++++++++- 2 files changed, 126 insertions(+), 4 deletions(-) diff --git a/src/app/actor.rs b/src/app/actor.rs index 519977e..1cad11c 100644 --- a/src/app/actor.rs +++ b/src/app/actor.rs @@ -202,7 +202,7 @@ async fn on_input( #[cfg(test)] mod tests { - use std::collections::HashMap; + use std::{collections::HashMap, sync::Arc}; use tokio::sync::mpsc; @@ -221,8 +221,22 @@ mod tests { env::MockEnvTrait, file_system::MockFileSystemTrait, shell::MockShellTrait, }, input::{event::InputEvent, handle::InputHandle, messages::InputMessage}, - lore::{application::handle::LoreApiHandle, domain::mailing_list::MailingList}, - render::handle::RenderHandle, + lore::{ + application::{ + actor::LoreApiActor, + cache::CacheTtl, + handle::LoreApiHandle, + service::LoreService, + }, + domain::mailing_list::MailingList, + infrastructure::{ + http_lore_client::{MockFeedGateway, MockListsGateway, MockPatchHtmlGateway}, + patchset_fetcher::MockPatchsetFetcher, + patchset_parser::MockPatchsetParser, + persistence::{MockMailingListsCacheStore, MockUserLoreStateStore}, + }, + }, + render::{actor::RenderActor, handle::RenderHandle, ShellRenderService}, terminal::{ actor::TerminalActor, messages::TerminalFrame, session::MockTerminalSessionApi, }, @@ -333,6 +347,93 @@ mod tests { assert!(result.is_ok()); } + fn spawn_real_lore_api(list: MailingList) -> LoreApiHandle { + let mut lists_store = MockMailingListsCacheStore::new(); + lists_store + .expect_load_available_lists() + .returning(move || Ok(vec![list.clone()])); + let mut user_state = MockUserLoreStateStore::new(); + user_state + .expect_load_bookmarked_patchsets() + .returning(|| Ok(vec![])); + user_state + .expect_load_reviewed_patchsets() + .returning(|| Ok(HashMap::new())); + + let service = LoreService::new( + Arc::new(MockListsGateway::new()), + Arc::new(MockFeedGateway::new()), + Arc::new(MockPatchHtmlGateway::new()), + Arc::new(lists_store), + Arc::new(user_state), + Arc::new(MockPatchsetFetcher::new()), + Arc::new(MockPatchsetParser::new()), + Arc::new(MockFileSystemTrait::new()), + Arc::new(MockShellTrait::new()), + CacheTtl::default(), + ); + LoreApiActor::spawn(service) + } + + fn spawn_real_render() -> RenderHandle { + RenderActor::spawn(Box::new(ShellRenderService::new(Arc::new( + MockShellTrait::new(), + )))) + } + + /// Verifies that AppActor, LoreApiActor, and RenderActor can be wired + /// together, go through a full bootstrap cycle, and all shut down cleanly + /// in the documented order when the input channel closes. + #[tokio::test(flavor = "multi_thread")] + async fn three_actor_lifecycle_stops_cleanly() { + let dummy_list = MailingList::new("test-list", "Test list"); + let lore_api = spawn_real_lore_api(dummy_list.clone()); + let render = spawn_real_render(); + + let bootstrap = lore_api + .get_bootstrap_data() + .await + .expect("bootstrap must succeed with mock infrastructure"); + assert_eq!(1, bootstrap.mailing_lists.len()); + + let mut env = MockEnvTrait::new(); + env.expect_which().returning(|_| true); + + let mut session = MockTerminalSessionApi::new(); + session + .expect_draw() + .withf(|frame| matches!(frame, TerminalFrame::Main(_))) + .times(1..) + .returning(|_| Ok(())); + let terminal_handle = TerminalActor::spawn(Box::new(session)); + let ui_handle = UiActor::spawn(); + + let app = App::new( + Box::new(NullConfigService), + bootstrap, + Box::new(MockFileSystemTrait::new()), + Box::new(MockShellTrait::new()), + Box::new(env), + lore_api.clone(), + render.clone(), + ) + .expect("App::new must succeed"); + + let (event_tx, event_rx) = mpsc::channel::(1); + let (input_tx, _input_rx) = mpsc::channel::(1); + let input_handle = InputHandle::new(input_tx); + + let handle = AppActor::spawn(app, terminal_handle, ui_handle, input_handle, event_rx); + + // Closing the input channel stops AppActor. + drop(event_tx); + assert!(handle.run_until_done().await.is_ok()); + + // Explicit shutdown in documented order: LoreAPI then Render. + lore_api.shutdown().await; + render.shutdown().await; + } + #[tokio::test(flavor = "multi_thread")] async fn missing_b4_causes_actor_to_return_dependencies_error() { let mut env = MockEnvTrait::new(); diff --git a/src/input/actor.rs b/src/input/actor.rs index 5e865ea..d95f0d4 100644 --- a/src/input/actor.rs +++ b/src/input/actor.rs @@ -134,7 +134,7 @@ mod tests { context::InputContext, event::{InputEvent, KeyInput, TerminalEvent}, }, - terminal::{actor::TerminalActor, session::MockTerminalSessionApi}, + terminal::{actor::TerminalActor, session::MockTerminalSessionApi, TerminalError}, }; use super::*; @@ -244,6 +244,27 @@ mod tests { assert!(sub_rx.recv().await.is_none()); } + /// Verifies that a terminal I/O error propagates through the event pump to + /// InputActor, causing InputActor to stop and its subscriber channel to close. + #[tokio::test] + async fn terminal_poll_error_stops_input_actor_and_closes_subscriber() { + let mut session = MockTerminalSessionApi::new(); + // First poll returns an error; the pump detects it and stops. + session + .expect_poll_event() + .times(1) + .returning(|_| Err(TerminalError::Session("simulated terminal failure".to_string()))); + + let (input_handle, _terminal_handle) = spawn_test_actor(session, mailing_list_context()); + let (sub_tx, mut sub_rx) = mpsc::channel::(8); + input_handle.subscribe_app(sub_tx).await.unwrap(); + + // When the pump stops due to the error, it drops event_tx. + // InputActor sees event_rx close and stops, dropping the subscriber sender. + // sub_rx.recv() therefore returns None. + assert!(sub_rx.recv().await.is_none()); + } + #[tokio::test] async fn popup_open_context_maps_escape_to_close_popup() { let mut session = MockTerminalSessionApi::new(); From 73ebd67b067df5a60dff3739bacd4429c0e48554 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Sat, 27 Jun 2026 15:35:57 -0300 Subject: [PATCH 09/10] docs: document hybrid actor architecture in README and module comments This commit documents the hybrid actor model where developers read code: module-level comments on each actor describe responsibility and handle boundaries, and README adds an Architecture section covering actor roles, communication rules, and the shutdown order after run_until_done. This commit is part of the architecture's refactoring phase 13. Signed-off-by: lorenzoberts --- README.md | 47 +++++++++++++++++++++++++++++++++++ src/app/actor.rs | 16 +++++++++--- src/app/mod.rs | 33 ++++++++++++++++++++---- src/app/updates.rs | 9 +++---- src/input/actor.rs | 21 +++++++++++++--- src/lore/application/actor.rs | 7 ++++++ src/render/actor.rs | 8 ++++++ src/terminal/actor.rs | 7 ++++++ src/ui/actor.rs | 10 ++++++++ 9 files changed, 139 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index 4b2ae7a..4e4f232 100644 --- a/README.md +++ b/README.md @@ -21,6 +21,53 @@ integrating it with the full `kw` suite for a more seamless developer experience. Check out the [kw repository](https://github.com/kworkflow/kworkflow/) to learn more. +## :building_construction: Architecture + +`patch-hub` uses a **hybrid actor model**: orchestration and domain boundaries +are actors communicating through typed handles and message channels, while +cross-cutting infrastructure (filesystem, shell, environment, HTTP) stays as +plain traits injected where needed. Observability (`tracing` + structured log +files) is a global layer initialized at startup, not an actor. + +### Actors + +| Actor | Handle | Role | +| --- | --- | --- | +| **App** | `AppHandle` | Central orchestrator: owns application state, dispatches screen flows, projects `AppViewModel`, drives the render/input loop | +| **LoreAPI** | `LoreApiHandle` | Lore domain: mailing lists, feeds, patchset details, bookmarks, reviewed state, git reply preparation | +| **Render** | `RenderHandle` | Patch/cover preview rendering via external tools (`bat`, `delta`, `diff-so-fancy`) | +| **Terminal** | `TerminalHandle` | Raw TUI session: draw, poll events, size, user-I/O setup | +| **Input** | `InputHandle` | Maps terminal events to semantic `InputEvent` values and forwards them to App | +| **UI** | `UiHandle` | Builds `UiScene` from `AppViewModel` for each frame | + +Configuration is **not** an actor: [`ConfigService`](src/config/service.rs) is +bootstrapped once and injected into `App` as a trait object. + +### Communication and boundaries + +- Cross-actor calls use **cloneable handles** and **typed messages** (`mpsc` + + `oneshot` for request/reply). Actors do not share mutable state. +- `AppState` holds domain/UI-flow state only; ratatui types and layout logic + stay in the UI actor. The app layer exposes [`AppViewModel`](src/app/view_model.rs) + as the presentation boundary. +- `InputActor` sits between `TerminalActor` and `AppActor`, breaking a direct + terminal ↔ app dependency cycle. + +### Startup and shutdown + +At startup (`main.rs`), actors are spawned and wired: Terminal and UI first, +then LoreAPI and Render, then App with an input channel subscribed from +`InputActor`. `AppActor::run_until_done()` blocks until the user exits. + +Shutdown order after the app loop finishes: + +1. **LoreAPI** and **Render** — no further data/render requests +2. **UI** — no further scene builds +3. **Terminal** — restored last so the screen stays usable during teardown + +`InputActor` stops when `AppActor` drops its handle; `AppActor` stops when the +input event channel closes. + ## :star: Features bail!("{e:#?}"), }; - debug!(msg_id, patches = details.raw_patches.len(), "rendering patchset preview"); + debug!( + msg_id, + patches = details.raw_patches.len(), + "rendering patchset preview" + ); let render_request = RenderPatchsetRequest::new( details.raw_patches.clone(), *self.state.config.patch_renderer(), @@ -335,7 +352,10 @@ impl App { let patches_to_reply = details.patches_to_reply.clone(); if let Some(true) = patchset_actions.get(&PatchsetAction::ReplyWithReviewedBy) { - debug!(msg_id = representative_patch.message_id().href, "executing reviewed-by reply"); + debug!( + msg_id = representative_patch.message_id().href, + "executing reviewed-by reply" + ); let mut successful_indexes = self .state .user_state @@ -439,8 +459,11 @@ impl App { .details .as_ref() .expect("invariant: details must be loaded before applying patchset") - .apply_patchset(&*self.services.fs, &*self.services.shell, &self.state.config) - { + .apply_patchset( + &*self.services.fs, + &*self.services.shell, + &self.state.config, + ) { Ok(msg) => popup::AppPopup::info("Patchset Apply Success", msg), Err(msg) => popup::AppPopup::info("Patchset Apply Fail", msg), }; diff --git a/src/app/updates.rs b/src/app/updates.rs index 1e378bf..f58c722 100644 --- a/src/app/updates.rs +++ b/src/app/updates.rs @@ -22,12 +22,9 @@ impl App { } } CurrentScreen::LatestPatchsets => { - let patchsets_state = self - .state - .lore - .latest_patchsets - .as_ref() - .expect("invariant: latest_patchsets must be initialised on LatestPatchsets screen"); + let patchsets_state = self.state.lore.latest_patchsets.as_ref().expect( + "invariant: latest_patchsets must be initialised on LatestPatchsets screen", + ); if patchsets_state.processed_patchsets_count() == 0 { let target_list = patchsets_state.target_list().to_string(); diff --git a/src/input/actor.rs b/src/input/actor.rs index d95f0d4..d76f8c9 100644 --- a/src/input/actor.rs +++ b/src/input/actor.rs @@ -1,3 +1,15 @@ +//! Input mediation actor: polls the terminal and maps raw events to semantic +//! [`InputEvent`](crate::input::event::InputEvent) values for the application. +//! +//! A dedicated pump subtask calls +//! [`TerminalHandle::poll_event`](crate::terminal::handle::TerminalHandle::poll_event) +//! so an in-flight poll is never abandoned when a control message wins the +//! select race. Mapped events are delivered to the subscriber channel +//! registered via +//! [`InputHandle::subscribe_app`](crate::input::handle::InputHandle::subscribe_app); +//! context updates from +//! [`InputHandle::update_context`](crate::input::handle::InputHandle::update_context) +//! change key bindings without restarting the pump. use std::time::Duration; use tokio::sync::mpsc; @@ -250,10 +262,11 @@ mod tests { async fn terminal_poll_error_stops_input_actor_and_closes_subscriber() { let mut session = MockTerminalSessionApi::new(); // First poll returns an error; the pump detects it and stops. - session - .expect_poll_event() - .times(1) - .returning(|_| Err(TerminalError::Session("simulated terminal failure".to_string()))); + session.expect_poll_event().times(1).returning(|_| { + Err(TerminalError::Session( + "simulated terminal failure".to_string(), + )) + }); let (input_handle, _terminal_handle) = spawn_test_actor(session, mailing_list_context()); let (sub_tx, mut sub_rx) = mpsc::channel::(8); diff --git a/src/lore/application/actor.rs b/src/lore/application/actor.rs index a6bfd73..3f41f5c 100644 --- a/src/lore/application/actor.rs +++ b/src/lore/application/actor.rs @@ -1,3 +1,10 @@ +//! Lore domain actor: serializes access to [`LoreService`] on a dedicated task. +//! +//! All lore I/O (mailing lists, feed pages, patchset details, bookmarks, +//! reviewed state, git reply preparation) goes through +//! [`LoreApiHandle`](crate::lore::application::handle::LoreApiHandle) as typed +//! request/reply messages. Heavy work runs on a blocking thread pool via +//! [`LoreApiActor::with_core`]; callers never touch [`LoreService`] directly. use std::ops::ControlFlow; use tokio::{ diff --git a/src/render/actor.rs b/src/render/actor.rs index cbadf63..65dc87e 100644 --- a/src/render/actor.rs +++ b/src/render/actor.rs @@ -1,3 +1,11 @@ +//! Render actor: serializes patch/cover preview rendering on a dedicated task. +//! +//! [`RenderHandle::render_patchset_preview`](crate::render::handle::RenderHandle::render_patchset_preview) +//! sends a [`RenderMessage`](crate::render::messages::RenderMessage) to this +//! actor, which delegates to a [`RenderServiceApi`](crate::render::RenderServiceApi) +//! implementation (typically [`ShellRenderService`](crate::render::ShellRenderService)) +//! on a blocking thread pool. Keeps shell subprocess work off the async runtime +//! and the UI thread. use std::ops::ControlFlow; use tokio::{ diff --git a/src/terminal/actor.rs b/src/terminal/actor.rs index 919fb02..ba2b324 100644 --- a/src/terminal/actor.rs +++ b/src/terminal/actor.rs @@ -1,3 +1,10 @@ +//! Terminal session actor: owns raw TUI I/O on a dedicated task. +//! +//! [`TerminalHandle`](crate::terminal::handle::TerminalHandle) exposes draw, +//! poll/read event, size, and user-I/O setup as typed messages. The session +//! implementation ([`TerminalSessionApi`](crate::terminal::session::TerminalSessionApi), +//! e.g. crossterm) is moved into the actor at spawn time so no other component +//! holds the terminal directly. use tokio::{ sync::{mpsc, oneshot}, task, diff --git a/src/ui/actor.rs b/src/ui/actor.rs index 4aaacea..032c426 100644 --- a/src/ui/actor.rs +++ b/src/ui/actor.rs @@ -1,3 +1,13 @@ +//! UI presentation actor: builds [`UiScene`](crate::ui::scene::UiScene) values +//! from [`AppViewModel`](crate::app::view_model::AppViewModel) on a dedicated task. +//! +//! [`AppActor`](crate::app::actor::AppActor) calls +//! [`UiHandle::build_scene`](crate::ui::handle::UiHandle::build_scene) each frame; +//! this actor runs [`UiCore::build_scene`](crate::ui::core::UiCore::build_scene) +//! and returns an owned scene for the terminal draw path. Presentation logic stays +//! out of [`AppState`](crate::app::state::AppState): the app layer projects domain +//! state into [`AppViewModel`](crate::app::view_model::AppViewModel) before +//! crossing this boundary. use std::ops::ControlFlow; use tokio::sync::{mpsc, oneshot}; From f1360cb73af75f9f30862dc05858b84a27cc7c88 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Sun, 28 Jun 2026 11:16:27 -0300 Subject: [PATCH 10/10] refactor: remove post-migration scaffolding and dead code This commit removes leftover placeholder types, unused error variants, and stale migration comments so the codebase reflects what is actually wired today. Test-only protocol surface is moved behind cfg(test), and error enums are trimmed to variants that production code constructs. This commit completes the architecture's refactoring phase 13. Signed-off-by: lorenzoberts --- src/app/errors.rs | 21 --------------------- src/app/popup.rs | 10 ---------- src/app/screens/latest.rs | 5 ----- src/app/state.rs | 2 +- src/config/errors.rs | 2 -- src/config/repository.rs | 14 -------------- src/config/service.rs | 2 +- src/config/state.rs | 3 +-- src/input/actor.rs | 7 ++++++- src/input/bindings.rs | 2 +- src/input/event.rs | 8 +------- src/input/handle.rs | 2 +- src/input/mapper.rs | 5 ----- src/input/messages.rs | 1 + src/lore/application/cache.rs | 9 +-------- src/lore/application/dto.rs | 5 +---- src/lore/application/service.rs | 5 ----- src/lore/domain/patchset.rs | 5 +---- src/render/trait.rs | 6 ------ src/terminal/actor.rs | 1 + src/terminal/handle.rs | 6 +----- src/terminal/messages.rs | 2 ++ src/ui/core.rs | 11 +++-------- src/ui/errors.rs | 7 ------- src/ui/mod.rs | 1 - src/ui/theme.rs | 7 ------- 26 files changed, 23 insertions(+), 126 deletions(-) delete mode 100644 src/ui/theme.rs diff --git a/src/app/errors.rs b/src/app/errors.rs index 40346ad..7ad8651 100644 --- a/src/app/errors.rs +++ b/src/app/errors.rs @@ -1,31 +1,10 @@ use thiserror::Error; -use crate::{lore::application::errors::LoreError, ui::errors::UiError}; - -#[allow(dead_code)] /// Errors surfaced by the application orchestration layer (`App`). /// /// Used as the typed boundary for `AppActor` messages and startup validation. #[derive(Debug, Error)] pub enum AppError { - #[error("invalid navigation state: {0}")] - InvalidState(String), - - #[error("lore error: {0}")] - Lore(#[from] LoreError), - - #[error("render error: {0}")] - Render(String), - - #[error("config error: {0}")] - Config(String), - - #[error("ui error: {0}")] - Ui(#[from] UiError), - - #[error("input error: {0}")] - Input(String), - #[error("missing required dependencies: {0}")] Dependencies(String), } diff --git a/src/app/popup.rs b/src/app/popup.rs index 053389f..139a393 100644 --- a/src/app/popup.rs +++ b/src/app/popup.rs @@ -150,16 +150,6 @@ impl AppPopup { _ => {} } } - - /// `(width_percent, height_percent)` used for the centred popup rect. - #[allow(dead_code)] - pub fn dimensions(&self) -> (u16, u16) { - match self { - AppPopup::Info { dimensions, .. } - | AppPopup::Help { dimensions, .. } - | AppPopup::ReviewTrailers { dimensions, .. } => *dimensions, - } - } } // --------------------------------------------------------------------------- diff --git a/src/app/screens/latest.rs b/src/app/screens/latest.rs index 589dc23..18e5dcb 100644 --- a/src/app/screens/latest.rs +++ b/src/app/screens/latest.rs @@ -39,11 +39,6 @@ impl LatestPatchsetsState { self.patchset_index } - #[allow(dead_code)] - pub fn page_size(&self) -> usize { - self.page_size - } - pub async fn fetch_current_page( &mut self, lore_api: &LoreApiHandle, diff --git a/src/app/state.rs b/src/app/state.rs index 0d90dc1..d741879 100644 --- a/src/app/state.rs +++ b/src/app/state.rs @@ -39,7 +39,7 @@ pub struct ConfigUiState { pub edit_config: Option, } -/// All application state grouped for the future App actor. +/// All application state grouped as the App actor's state. #[derive(Clone)] pub struct AppState { pub navigation: NavigationState, diff --git a/src/config/errors.rs b/src/config/errors.rs index b049b60..72d49af 100644 --- a/src/config/errors.rs +++ b/src/config/errors.rs @@ -4,8 +4,6 @@ use crate::infrastructure::file_system::FileSystemError; #[derive(Debug, Error)] pub enum ConfigError { - #[error("failed to load config: {0}")] - Load(String), #[error("failed to save config: {0}")] Save(String), #[error("invalid page size: {0}")] diff --git a/src/config/repository.rs b/src/config/repository.rs index 9f81cf7..5be91d5 100644 --- a/src/config/repository.rs +++ b/src/config/repository.rs @@ -6,8 +6,6 @@ use crate::config::DEFAULT_CONFIG_PATH_SUFFIX; use crate::infrastructure::{env::EnvTrait, file_system::FileSystemTrait}; pub trait ConfigRepository: Send + Sync { - #[allow(dead_code)] - fn load(&self) -> Result; fn save(&self, state: &ConfigState) -> Result<(), ConfigError>; } @@ -45,18 +43,6 @@ impl JsonConfigRepository { } impl ConfigRepository for JsonConfigRepository { - fn load(&self) -> Result { - let path = Path::new(&self.config_path); - if !self.fs.is_file(path) { - return Err(ConfigError::Load("config file not found".into())); - } - let contents = self - .fs - .read_to_string(path) - .map_err(|e| ConfigError::Load(e.to_string()))?; - serde_json::from_str(&contents).map_err(|e| ConfigError::Load(e.to_string())) - } - fn save(&self, state: &ConfigState) -> Result<(), ConfigError> { let config_path = Path::new(&self.config_path); if let Some(parent_dir) = Path::parent(config_path) { diff --git a/src/config/service.rs b/src/config/service.rs index 78c8139..200470a 100644 --- a/src/config/service.rs +++ b/src/config/service.rs @@ -9,7 +9,7 @@ use crate::config::state::{normalize_derived_paths, ConfigState}; use crate::config::update::{ConfigUpdateDraft, ValidatedConfigUpdate}; use crate::infrastructure::{env::EnvTrait, file_system::FileSystemTrait}; -/// Public surface for configuration (maps to a future `ConfigActor` protocol). +/// Public surface for configuration. pub trait ConfigServiceApi: Send + Sync { fn snapshot(&self) -> ConfigSnapshot; fn validate_update( diff --git a/src/config/state.rs b/src/config/state.rs index f36dc6f..a71bbb2 100644 --- a/src/config/state.rs +++ b/src/config/state.rs @@ -79,7 +79,7 @@ impl ConfigState { } } - #[allow(dead_code)] + #[cfg(test)] pub fn page_size(&self) -> usize { self.page_size } @@ -219,7 +219,6 @@ impl ConfigSnapshot { self.kernel_trees.keys().collect::>() } - #[allow(dead_code)] pub fn get_kernel_tree(&self, kernel_tree_id: &str) -> Option<&KernelTree> { self.kernel_trees.get(kernel_tree_id) } diff --git a/src/input/actor.rs b/src/input/actor.rs index d76f8c9..3710720 100644 --- a/src/input/actor.rs +++ b/src/input/actor.rs @@ -96,7 +96,12 @@ impl InputActor { tracing::debug!(?context, "input context updated"); self.context = context; } - Some(InputMessage::Shutdown) | None => { + #[cfg(test)] + Some(InputMessage::Shutdown) => { + tracing::info!("input actor stopping"); + break; + } + None => { tracing::info!("input actor stopping"); break; } diff --git a/src/input/bindings.rs b/src/input/bindings.rs index e19fef1..e5cb2c8 100644 --- a/src/input/bindings.rs +++ b/src/input/bindings.rs @@ -1,6 +1,6 @@ use std::time::Duration; -/// Fixed input timing values for the Phase 8 protocol. +/// Fixed input timing values. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct KeyBindings { pub chord_timeout: Duration, diff --git a/src/input/event.rs b/src/input/event.rs index b997f93..b887e16 100644 --- a/src/input/event.rs +++ b/src/input/event.rs @@ -4,12 +4,7 @@ use ratatui::crossterm::event::{KeyCode, KeyEvent, KeyEventKind, KeyModifiers}; #[derive(Debug, Clone, PartialEq, Eq)] pub enum TerminalEvent { Key(KeyInput), - Resize { - width: u16, - height: u16, - }, - #[allow(dead_code)] // Reserved for future non-blocking refresh loops. - Tick, + Resize { width: u16, height: u16 }, } /// Key data kept close to crossterm while avoiding `KeyEvent` outside input. @@ -92,7 +87,6 @@ pub enum InputEvent { TogglePreviewFullscreen, ShowReviewTrailers, Resize { width: u16, height: u16 }, - Tick, } #[derive(Debug, Clone, Copy, PartialEq, Eq)] diff --git a/src/input/handle.rs b/src/input/handle.rs index 7eb73de..2fc76c7 100644 --- a/src/input/handle.rs +++ b/src/input/handle.rs @@ -38,7 +38,7 @@ impl InputHandle { /// /// Dropping all clones of the handle achieves the same effect because the /// actor's receive channel closes when its last sender is gone. - #[allow(dead_code)] + #[cfg(test)] pub async fn shutdown(&self) -> Result<(), InputError> { self.send(InputMessage::Shutdown).await } diff --git a/src/input/mapper.rs b/src/input/mapper.rs index 1bba16d..5166dc7 100644 --- a/src/input/mapper.rs +++ b/src/input/mapper.rs @@ -41,7 +41,6 @@ impl InputMapper { match event { TerminalEvent::Key(key) => self.map_key_input(key, context), TerminalEvent::Resize { width, height } => Some(InputEvent::Resize { width, height }), - TerminalEvent::Tick => Some(InputEvent::Tick), } } @@ -387,9 +386,5 @@ mod tests { height: 40, }) ); - assert_eq!( - mapper.map_terminal_event(TerminalEvent::Tick, &context), - Some(InputEvent::Tick) - ); } } diff --git a/src/input/messages.rs b/src/input/messages.rs index b496e28..cb0bca9 100644 --- a/src/input/messages.rs +++ b/src/input/messages.rs @@ -10,5 +10,6 @@ pub enum InputMessage { /// terminal event with current application state. UpdateContext { context: InputContext }, /// Requests a clean shutdown of the actor's event loop. + #[cfg(test)] Shutdown, } diff --git a/src/lore/application/cache.rs b/src/lore/application/cache.rs index f1f40c2..591b1c7 100644 --- a/src/lore/application/cache.rs +++ b/src/lore/application/cache.rs @@ -53,10 +53,7 @@ impl Default for CacheTtl { // ── Generic cache policy trait ──────────────────────────────────────────────── -/// Homogeneous interface shared by all three cache stores. -/// -/// Not yet used by any concrete implementor; provided as a documented -/// extension point for future phases. +/// Uniform cache interface (`get`, `put`, `invalidate`, `clear`) for Lore cache stores. #[allow(dead_code)] pub trait CachePolicy { fn get(&self, key: &K) -> Option<&V>; @@ -94,9 +91,6 @@ impl MailingListsCacheEntry { pub struct FeedCacheEntry { pub index: PatchFeedIndex, pub fetched_at: SystemTime, - /// `true` once the gateway has returned `EndOfFeed` for this list. - #[allow(dead_code)] - pub complete: bool, } impl FeedCacheEntry { @@ -104,7 +98,6 @@ impl FeedCacheEntry { FeedCacheEntry { index, fetched_at: SystemTime::now(), - complete: false, } } diff --git a/src/lore/application/dto.rs b/src/lore/application/dto.rs index 69b8ed7..c26bb0e 100644 --- a/src/lore/application/dto.rs +++ b/src/lore/application/dto.rs @@ -1,6 +1,6 @@ use std::collections::HashSet; -use crate::lore::domain::patch::{Author, Patch}; +use crate::lore::domain::patch::Author; /// Per-patch tag summary extracted from a raw patch's cover section. #[derive(Clone)] @@ -12,9 +12,6 @@ pub struct PatchTagSummary { /// Complete data for displaying and acting on a patchset in the UI. pub struct PatchsetDetails { - /// Kept for future actor-model phases that will need the representative patch. - #[allow(dead_code)] - pub representative_patch: Patch, /// Path on disk to the downloaded `.mbx` file (needed for `git am`). pub patchset_path: String, /// Raw text of each individual patch (cover letter first, if present). diff --git a/src/lore/application/service.rs b/src/lore/application/service.rs index bf51832..38bfec7 100644 --- a/src/lore/application/service.rs +++ b/src/lore/application/service.rs @@ -230,9 +230,6 @@ impl LoreService { entry.index.advance_offset(); } Err(LoreHttpError::EndOfFeed) => { - if let Some(entry) = self.cache.feeds.get_mut(target_list) { - entry.complete = true; - } break; } Err(e) => return Err(LoreError::Http(e)), @@ -268,7 +265,6 @@ impl LoreService { } else { tracing::debug!(msg_id = %key.message_id, "patchset cache: hit"); return Ok(PatchsetDetails { - representative_patch: representative_patch.clone(), patchset_path: entry.patchset_path.clone(), raw_patches: entry.raw_patches.clone(), tag_summary: entry.tag_summary.clone(), @@ -307,7 +303,6 @@ impl LoreService { } Ok(PatchsetDetails { - representative_patch: representative_patch.clone(), patchset_path, raw_patches, tag_summary, diff --git a/src/lore/domain/patchset.rs b/src/lore/domain/patchset.rs index 2803b1d..92140c8 100644 --- a/src/lore/domain/patchset.rs +++ b/src/lore/domain/patchset.rs @@ -8,7 +8,6 @@ const LORE_PAGE_SIZE: usize = 200; /// /// Replaces the state that was previously mixed into [`LoreSession`]. pub struct PatchFeedIndex { - // TODO: used by the actor model (Phase 6) to identify which list this index belongs to. #[allow(dead_code)] target_list: String, next_offset: usize, @@ -28,7 +27,6 @@ impl PatchFeedIndex { } } - // TODO: used by the actor model (Phase 6) for random-access patch lookup by message ID. #[allow(dead_code)] pub fn target_list(&self) -> &str { &self.target_list @@ -42,8 +40,7 @@ impl PatchFeedIndex { &self.representative_patch_ids } - // TODO: used by the actor model (Phase 6) for random-access patch lookup by message ID. - #[allow(dead_code)] + #[cfg(test)] pub fn get_patch(&self, id: &str) -> Option<&Patch> { self.patches_by_id.get(id) } diff --git a/src/render/trait.rs b/src/render/trait.rs index 7b07ac2..1d0c115 100644 --- a/src/render/trait.rs +++ b/src/render/trait.rs @@ -4,14 +4,8 @@ use thiserror::Error; use super::dto::{RenderPatchsetRequest, RenderedPatchsetPreview}; /// Failure while running an external preview renderer (bat, delta, etc.). -/// -/// Reserved for future strict error propagation; [`super::ShellRenderService`] -/// currently falls back to raw text and returns `Ok`. -#[allow(dead_code)] #[derive(Debug, Error)] pub enum RenderError { - #[error("render failed: {0}")] - Failed(String), #[error("render actor unavailable: {0}")] ActorUnavailable(String), } diff --git a/src/terminal/actor.rs b/src/terminal/actor.rs index ba2b324..c0746af 100644 --- a/src/terminal/actor.rs +++ b/src/terminal/actor.rs @@ -62,6 +62,7 @@ impl TerminalActor { .and_then(|result| result); send_terminal_reply(message_name, reply, result); } + #[cfg(test)] TerminalMessage::ReadEvent { reply } => { let result = self .with_session(|session| session.read_event()) diff --git a/src/terminal/handle.rs b/src/terminal/handle.rs index f9d42e5..040a533 100644 --- a/src/terminal/handle.rs +++ b/src/terminal/handle.rs @@ -27,11 +27,7 @@ impl TerminalHandle { } /// Reads the next raw terminal event, blocking until one arrives. - /// - /// The runtime event loop now receives input through [`InputHandle`] rather - /// than calling this directly. This method is kept for potential future use - /// in interactive sub-flows. - #[allow(dead_code)] + #[cfg(test)] pub async fn read_event(&self) -> TerminalResult> { self.request_result(|reply| TerminalMessage::ReadEvent { reply }) .await diff --git a/src/terminal/messages.rs b/src/terminal/messages.rs index 72272d4..7e4a9e3 100644 --- a/src/terminal/messages.rs +++ b/src/terminal/messages.rs @@ -22,6 +22,7 @@ pub enum TerminalMessage { frame: TerminalFrame, reply: oneshot::Sender>, }, + #[cfg(test)] ReadEvent { reply: oneshot::Sender>>, }, @@ -52,6 +53,7 @@ impl TerminalMessage { pub fn name(&self) -> &'static str { match self { TerminalMessage::Draw { .. } => "Draw", + #[cfg(test)] TerminalMessage::ReadEvent { .. } => "ReadEvent", TerminalMessage::PollEvent { .. } => "PollEvent", TerminalMessage::SetupUserIo { .. } => "SetupUserIo", diff --git a/src/ui/core.rs b/src/ui/core.rs index cc06b3a..4240fd4 100644 --- a/src/ui/core.rs +++ b/src/ui/core.rs @@ -2,25 +2,20 @@ //! //! `UiCore` transforms an [`AppViewModel`] into a paint-ready [`UiScene`] by //! dispatching to per-screen builders and composing the navigation bar and -//! optional popup. It holds a [`UiTheme`] so future styling policy can be -//! applied centrally without touching individual screen painters. +//! optional popup. use crate::app::view_model::{AppViewModel, ScreenViewModel}; use crate::ui::{ errors::UiError, scene::{NavigationBarScene, UiBody, UiScene}, screens, - theme::UiTheme, }; -pub struct UiCore { - #[allow(dead_code)] - theme: UiTheme, -} +pub struct UiCore; impl UiCore { pub fn new() -> Self { - Self { theme: UiTheme } + Self } /// Transform `vm` into a fully projected [`UiScene`] ready for painting. diff --git a/src/ui/errors.rs b/src/ui/errors.rs index c6427bf..c3a49a0 100644 --- a/src/ui/errors.rs +++ b/src/ui/errors.rs @@ -1,13 +1,6 @@ -#![allow(dead_code)] /// Errors produced by the UI actor boundary. #[derive(thiserror::Error, Debug)] pub enum UiError { #[error("failed to build scene: {0}")] Build(String), - - #[error("invalid screen state for UI: {0}")] - InvalidState(String), - - #[error("ui actor unavailable: {0}")] - ActorUnavailable(String), } diff --git a/src/ui/mod.rs b/src/ui/mod.rs index 01196ba..2f7e0a5 100644 --- a/src/ui/mod.rs +++ b/src/ui/mod.rs @@ -7,4 +7,3 @@ pub mod messages; pub mod painter; pub mod scene; mod screens; -pub mod theme; diff --git a/src/ui/theme.rs b/src/ui/theme.rs deleted file mode 100644 index ad638d7..0000000 --- a/src/ui/theme.rs +++ /dev/null @@ -1,7 +0,0 @@ -#![allow(dead_code)] -/// Visual theme configuration for the UI actor. -/// -/// Holds styling and layout policy. Kept minimal in phase 11; extended when -/// per-theme rendering is needed. -#[derive(Default, Clone, Debug)] -pub struct UiTheme;