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 , - 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 +147,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 - } - Some(AppMessage::Input { .. }) | 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(()) } @@ -252,7 +213,7 @@ async fn on_input( #[cfg(test)] mod tests { - use std::collections::HashMap; + use std::{collections::HashMap, sync::Arc}; use tokio::sync::mpsc; @@ -270,9 +231,20 @@ mod tests { infrastructure::{ env::MockEnvTrait, file_system::MockFileSystemTrait, shell::MockShellTrait, }, - input::{handle::InputHandle, messages::InputMessage}, - lore::{application::handle::LoreApiHandle, domain::mailing_list::MailingList}, - render::handle::RenderHandle, + input::{event::InputEvent, handle::InputHandle, messages::InputMessage}, + 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, }, @@ -353,7 +325,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 +336,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,11 +348,100 @@ 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()); } + 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/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/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/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/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 50bb445..0000000 --- a/src/app/messages.rs +++ /dev/null @@ -1,51 +0,0 @@ -use std::ops::ControlFlow; - -use tokio::sync::oneshot; - -use crate::{ - app::{errors::AppError, view_model::AppViewModel}, - input::{context::InputContext, event::InputEvent}, -}; - -/// 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>, - }, - - /// 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, - }, - - /// 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::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..d9f5599 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -1,20 +1,28 @@ +//! Application orchestration: state, screen flows, view-model projection, and the +//! central [`AppActor`](crate::app::actor::AppActor) run loop. +//! +//! [`App`] holds [`AppState`] (navigation, lore UI state, user data, config +//! snapshot) and [`AppServices`] (typed handles to +//! [`LoreApiHandle`](crate::lore::application::handle::LoreApiHandle), +//! [`RenderHandle`](crate::render::handle::RenderHandle), plus injected +//! infrastructure traits). Screen-specific input is dispatched from +//! [`AppActor`](crate::app::actor::AppActor) into [`crate::app::flows`]; +//! presentation data crosses the UI boundary only through [`AppViewModel`] via +//! [`App::present`]. pub mod actor; -pub mod commands; pub mod errors; 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; -pub mod transitions; 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; @@ -158,9 +166,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(()) } @@ -168,11 +184,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`]. @@ -194,7 +217,9 @@ impl App { .lore .latest_patchsets .as_ref() - .unwrap() + .expect( + "invariant: latest_patchsets must be initialised before opening details", + ) .get_selected_patchset(); if !self .state @@ -209,6 +234,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 @@ -216,10 +244,18 @@ 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(), @@ -232,13 +268,14 @@ 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, rendered_preview, is_patchset_bookmarked, self.state.navigation.current_screen.clone(), - )?); + )); Ok(B4Result::PatchFound) } @@ -254,23 +291,33 @@ 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(()) } 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; + 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 @@ -288,17 +335,27 @@ impl App { ) .await .map_err(|e| eyre!("{e:#?}"))?; + debug!(msg_id, "bookmark state persisted"); Ok(()) } 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(); 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 @@ -371,11 +428,15 @@ 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 .as_mut() - .unwrap() + .expect("invariant: details must be loaded before resetting reply action") .reset_reply_with_reviewed_by_action(); } Ok(()) @@ -387,15 +448,22 @@ 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, - ) { + debug!("applying patchset via git-am"); + 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), }; @@ -406,7 +474,7 @@ impl App { .lore .details .as_mut() - .unwrap() + .expect("invariant: details must be loaded before toggling apply action") .toggle_apply_action(); } } @@ -423,10 +491,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(()) } 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/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/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/screens/latest.rs b/src/app/screens/latest.rs index 53c003d..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, @@ -94,7 +89,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/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/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, -} diff --git a/src/app/updates.rs b/src/app/updates.rs index 2084b08..f58c722 100644 --- a/src/app/updates.rs +++ b/src/app/updates.rs @@ -22,7 +22,9 @@ 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/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, 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 2c7f587..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>; } @@ -15,7 +13,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 ) }) @@ -44,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 5e865ea..3710720 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; @@ -84,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; } @@ -134,7 +151,7 @@ mod tests { context::InputContext, event::{InputEvent, KeyInput, TerminalEvent}, }, - terminal::{actor::TerminalActor, session::MockTerminalSessionApi}, + terminal::{actor::TerminalActor, session::MockTerminalSessionApi, TerminalError}, }; use super::*; @@ -244,6 +261,28 @@ 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(); 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/actor.rs b/src/lore/application/actor.rs index aa4c8ab..3f41f5c 100644 --- a/src/lore/application/actor.rs +++ b/src/lore/application/actor.rs @@ -1,3 +1,12 @@ +//! 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::{ sync::{mpsc, oneshot}, task, @@ -35,12 +44,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 +63,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 +72,7 @@ impl LoreApiActor { .await .and_then(|result| result); send_lore_reply(message_name, reply, result); + ControlFlow::Continue(()) } LoreApiMessage::FetchFeedPage { target_list, @@ -82,6 +95,7 @@ impl LoreApiActor { .await .and_then(|result| result); send_lore_reply(message_name, reply, result); + ControlFlow::Continue(()) } LoreApiMessage::FetchPatchsetDetails { representative_patch, @@ -100,14 +114,7 @@ impl LoreApiActor { .await .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); + ControlFlow::Continue(()) } LoreApiMessage::SaveBookmarks { bookmarks, reply } => { tracing::debug!(count = bookmarks.len(), "saving bookmarked patchsets"); @@ -116,14 +123,7 @@ impl LoreApiActor { .await .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); + ControlFlow::Continue(()) } LoreApiMessage::SaveReviewed { reviewed, reply } => { tracing::debug!(patchsets = reviewed.len(), "saving reviewed patchsets"); @@ -132,6 +132,7 @@ impl LoreApiActor { .await .and_then(|result| result); send_lore_reply(message_name, reply, result); + ControlFlow::Continue(()) } LoreApiMessage::GetGitSignature { git_repo_path, @@ -142,6 +143,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, @@ -172,6 +174,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/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/handle.rs b/src/lore/application/handle.rs index 727cfc1..5b739e9 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>, @@ -107,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 6926c43..81ba640 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>, @@ -68,6 +60,7 @@ pub enum LoreApiMessage { git_send_email_options: String, reply: oneshot::Sender>>, }, + Shutdown, } impl LoreApiMessage { @@ -77,12 +70,11 @@ 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", + LoreApiMessage::Shutdown => "Shutdown", } } } 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/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 3b9cb9f..65dc87e 100644 --- a/src/render/actor.rs +++ b/src/render/actor.rs @@ -1,3 +1,13 @@ +//! 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::{ sync::{mpsc, oneshot}, task, @@ -6,7 +16,7 @@ use tokio::{ use crate::render::{ handle::RenderHandle, messages::{RenderMessage, RenderResult}, - RenderError, RenderPatchsetRequest, RenderServiceApi, + RenderError, RenderServiceApi, }; pub const DEFAULT_RENDER_CHANNEL_SIZE: usize = 32; @@ -37,12 +47,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,35 +71,11 @@ impl RenderActor { .await .and_then(|result| result); send_render_reply(message_name, reply, result); + ControlFlow::Continue(()) } - 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); + RenderMessage::Shutdown => { + tracing::debug!("render actor shutting down"); + ControlFlow::Break(()) } } } @@ -170,38 +158,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..8a96414 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)] @@ -20,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, @@ -28,21 +32,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..c231fab 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,14 @@ pub enum RenderMessage { request: RenderPatchsetRequest, reply: oneshot::Sender>, }, - RenderSinglePatch { - raw_patch: String, - patch_renderer: PatchRenderer, - cover_renderer: CoverRenderer, - reply: oneshot::Sender>, - }, + Shutdown, } impl RenderMessage { pub fn name(&self) -> &'static str { match self { RenderMessage::RenderPatchsetPreview { .. } => "RenderPatchsetPreview", - RenderMessage::RenderSinglePatch { .. } => "RenderSinglePatch", + RenderMessage::Shutdown => "Shutdown", } } } 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 919fb02..c0746af 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, @@ -55,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/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}; 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;