From 62f669e159221090e36bb9ded1d5b8b166929a60 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 9 Jul 2026 14:50:46 -0300 Subject: [PATCH 01/18] refactor(input): support explicit production shutdown This commit makes InputActor shutdown part of the production lifecycle instead of a test-only API. main retains a cloned input handle and shuts the actor down explicitly after AppActor exits, so the documented teardown order includes the input pump rather than relying on handle drops. This commit is part of the architecture's refactoring phase 15. Signed-off-by: lorenzoberts --- src/input/actor.rs | 1 - src/input/handle.rs | 1 - src/input/messages.rs | 1 - src/main.rs | 16 +++++++++++----- 4 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/input/actor.rs b/src/input/actor.rs index 3710720..999a8a4 100644 --- a/src/input/actor.rs +++ b/src/input/actor.rs @@ -96,7 +96,6 @@ impl InputActor { tracing::debug!(?context, "input context updated"); self.context = context; } - #[cfg(test)] Some(InputMessage::Shutdown) => { tracing::info!("input actor stopping"); break; diff --git a/src/input/handle.rs b/src/input/handle.rs index 2fc76c7..fef224d 100644 --- a/src/input/handle.rs +++ b/src/input/handle.rs @@ -38,7 +38,6 @@ 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. - #[cfg(test)] pub async fn shutdown(&self) -> Result<(), InputError> { self.send(InputMessage::Shutdown).await } diff --git a/src/input/messages.rs b/src/input/messages.rs index cb0bca9..b496e28 100644 --- a/src/input/messages.rs +++ b/src/input/messages.rs @@ -10,6 +10,5 @@ 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/main.rs b/src/main.rs index cb8de29..a09e68c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -128,14 +128,16 @@ async fn main() -> color_eyre::Result<()> { .subscribe_app(app_input_tx) .await .map_err(|e| eyre!("{e}"))?; + let input_shutdown_handle = input_handle.clone(); // Shutdown ordering: // 1. AppActor — exits when the user quits (input channel closes) - // 2. ConfigActor — no further configuration requests once App is gone - // 3. LoreApiActor — no further requests once App is gone - // 4. RenderActor — no further requests once App is gone - // 5. UiActor — no further scene builds once App is gone - // 6. TerminalActor — restores the terminal last so the screen stays usable + // 2. InputActor — no further terminal input is needed once App is gone + // 3. ConfigActor — no further configuration requests once App is gone + // 4. LoreApiActor — no further requests once App is gone + // 5. RenderActor — no further requests once App is gone + // 6. UiActor — no further scene builds once App is gone + // 7. TerminalActor — restores the terminal last so the screen stays usable // during the steps above AppActor::spawn( app, @@ -146,6 +148,10 @@ async fn main() -> color_eyre::Result<()> { ) .run_until_done() .await?; + input_shutdown_handle + .shutdown() + .await + .map_err(|e| eyre!("{e}"))?; config_handle.shutdown().await; lore_api.shutdown().await; render.shutdown().await; From 7cd29970b8e93a84bb5077b4f93145753f1aae94 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 9 Jul 2026 14:52:58 -0300 Subject: [PATCH 02/18] refactor(app): surface patchset open failures This commit handles patchset-open results explicitly in the latest and bookmarked flows instead of ignoring unexpected errors. PatchNotFound keeps its existing popup while other Lore, render, or parser failures surface as a generic error popup, leaving the user on the originating list screen. This commit is part of the architecture's refactoring phase 15. Signed-off-by: lorenzoberts --- src/app/flows/bookmarked.rs | 19 ++----- src/app/flows/latest.rs | 19 ++----- src/app/flows/mod.rs | 1 + src/app/flows/open_patchset.rs | 93 ++++++++++++++++++++++++++++++++++ 4 files changed, 102 insertions(+), 30 deletions(-) create mode 100644 src/app/flows/open_patchset.rs diff --git a/src/app/flows/bookmarked.rs b/src/app/flows/bookmarked.rs index b91cbd2..1937af1 100644 --- a/src/app/flows/bookmarked.rs +++ b/src/app/flows/bookmarked.rs @@ -1,10 +1,12 @@ use tracing::debug; use crate::{ - app::{loading::LoadingIndicator, popup::AppPopup, screens::CurrentScreen, App, B4Result}, + app::{loading::LoadingIndicator, popup::AppPopup, screens::CurrentScreen, App}, input::event::InputEvent, }; +use super::open_patchset::apply_open_patchset_result; + pub async fn handle_bookmarked_patchsets( app: &mut App, input: InputEvent, @@ -39,20 +41,7 @@ pub async fn handle_bookmarked_patchsets( // 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); - } - B4Result::PatchNotFound(err_cause) => { - app.state.popup = Some(AppPopup::info( - "Error", - format!("The selected patchset couldn't be retrieved.\nReason: {err_cause}\nPlease choose another patchset."), - )); - app.set_current_screen(CurrentScreen::BookmarkedPatchsets); - } - } - } + apply_open_patchset_result(app, CurrentScreen::BookmarkedPatchsets, result); } _ => {} } diff --git a/src/app/flows/latest.rs b/src/app/flows/latest.rs index 2193243..9ef3051 100644 --- a/src/app/flows/latest.rs +++ b/src/app/flows/latest.rs @@ -1,10 +1,12 @@ use tracing::debug; use crate::{ - app::{loading::LoadingIndicator, popup::AppPopup, screens::CurrentScreen, App, B4Result}, + app::{loading::LoadingIndicator, popup::AppPopup, screens::CurrentScreen, App}, input::event::InputEvent, }; +use super::open_patchset::apply_open_patchset_result; + pub async fn handle_latest_patchsets( app: &mut App, input: InputEvent, @@ -71,20 +73,7 @@ pub async fn handle_latest_patchsets( loading.start("Loading patchset".to_string()); let result = app.open_patchset_details().await; loading.stop()?; - if let Ok(b4_result) = result { - match b4_result { - B4Result::PatchFound => { - app.set_current_screen(CurrentScreen::PatchsetDetails); - } - B4Result::PatchNotFound(err_cause) => { - app.state.popup = Some(AppPopup::info( - "Error", - format!("The selected patchset couldn't be retrieved.\nReason: {err_cause}\nPlease choose another patchset."), - )); - app.set_current_screen(CurrentScreen::LatestPatchsets); - } - } - } + apply_open_patchset_result(app, CurrentScreen::LatestPatchsets, result); } _ => {} } diff --git a/src/app/flows/mod.rs b/src/app/flows/mod.rs index fdc72d8..ae496cc 100644 --- a/src/app/flows/mod.rs +++ b/src/app/flows/mod.rs @@ -3,3 +3,4 @@ pub(crate) mod details_actions; pub(crate) mod edit_config; pub(crate) mod latest; pub(crate) mod mail_list; +mod open_patchset; diff --git a/src/app/flows/open_patchset.rs b/src/app/flows/open_patchset.rs new file mode 100644 index 0000000..fc4d313 --- /dev/null +++ b/src/app/flows/open_patchset.rs @@ -0,0 +1,93 @@ +use crate::app::{popup::AppPopup, screens::CurrentScreen, App, B4Result}; + +#[derive(Debug, PartialEq)] +enum OpenPatchsetAction { + ShowDetails, + ShowError { + origin: CurrentScreen, + body: String, + }, +} + +pub(super) fn apply_open_patchset_result( + app: &mut App, + origin: CurrentScreen, + result: color_eyre::Result, +) { + match resolve_open_patchset_result(origin, result) { + OpenPatchsetAction::ShowDetails => { + app.set_current_screen(CurrentScreen::PatchsetDetails); + } + OpenPatchsetAction::ShowError { origin, body } => { + app.state.popup = Some(AppPopup::info("Error", body)); + app.set_current_screen(origin); + } + } +} + +fn resolve_open_patchset_result( + origin: CurrentScreen, + result: color_eyre::Result, +) -> OpenPatchsetAction { + match result { + Ok(B4Result::PatchFound) => OpenPatchsetAction::ShowDetails, + Ok(B4Result::PatchNotFound(err_cause)) => OpenPatchsetAction::ShowError { + origin, + body: format!( + "The selected patchset couldn't be retrieved.\nReason: {err_cause}\nPlease choose another patchset." + ), + }, + Err(error) => OpenPatchsetAction::ShowError { + origin, + body: format!("The selected patchset couldn't be opened.\nReason: {error:#}"), + }, + } +} + +#[cfg(test)] +mod tests { + use color_eyre::eyre::eyre; + + use super::*; + + #[test] + fn patch_found_opens_details() { + let action = resolve_open_patchset_result( + CurrentScreen::LatestPatchsets, + Ok(B4Result::PatchFound), + ); + + assert_eq!(OpenPatchsetAction::ShowDetails, action); + } + + #[test] + fn patch_not_found_shows_retrieval_error_on_origin_screen() { + let action = resolve_open_patchset_result( + CurrentScreen::BookmarkedPatchsets, + Ok(B4Result::PatchNotFound("not found by b4".to_string())), + ); + + assert_eq!( + OpenPatchsetAction::ShowError { + origin: CurrentScreen::BookmarkedPatchsets, + body: "The selected patchset couldn't be retrieved.\nReason: not found by b4\nPlease choose another patchset.".to_string(), + }, + action, + ); + } + + #[test] + fn unexpected_error_shows_open_error_on_origin_screen() { + let action = resolve_open_patchset_result( + CurrentScreen::LatestPatchsets, + Err(eyre!("render actor unavailable")), + ); + + let OpenPatchsetAction::ShowError { origin, body } = action else { + panic!("unexpected errors should show an error popup"); + }; + assert_eq!(CurrentScreen::LatestPatchsets, origin); + assert!(body.contains("The selected patchset couldn't be opened.")); + assert!(body.contains("render actor unavailable")); + } +} From 301dff3bc0a77f8760e9267c6f5fad599b813f1a Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 9 Jul 2026 14:54:56 -0300 Subject: [PATCH 03/18] refactor(startup): propagate lore bootstrap errors This commit stops defaulting Lore bootstrap to empty state when the actor request fails. Bootstrap errors now propagate through startup with context, so channel failures and unexpected bootstrap issues are visible instead of appearing as an app with no mailing lists, bookmarks, or reviewed state. This commit is part of the architecture's refactoring phase 15. Signed-off-by: lorenzoberts --- src/main.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/main.rs b/src/main.rs index a09e68c..c7ea6d4 100644 --- a/src/main.rs +++ b/src/main.rs @@ -107,7 +107,10 @@ async fn main() -> color_eyre::Result<()> { shell_arc.clone(), CacheTtl::default(), )); - let bootstrap = lore_api.get_bootstrap_data().await.unwrap_or_default(); + let bootstrap = lore_api + .get_bootstrap_data() + .await + .map_err(|error| eyre!("failed to bootstrap Lore data: {error}"))?; let app = App::new( config_handle From f59237de15634795eb1b969e925dee40b460c1d5 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 9 Jul 2026 14:56:44 -0300 Subject: [PATCH 04/18] refactor(startup): check dependencies before terminal init This commit moves external dependency validation out of AppActor and into startup before the terminal enters raw mode. Missing b4 remains fatal while missing optional render tools still log warnings, preserving existing policy while failing fast before terminal initialization. This commit is part of the architecture's refactoring phase 15. Signed-off-by: lorenzoberts --- src/app/actor.rs | 102 +-------------------------------- src/app/dependencies.rs | 123 ++++++++++++++++++++++++++++++++++++++++ src/app/mod.rs | 1 + src/main.rs | 4 +- 4 files changed, 130 insertions(+), 100 deletions(-) create mode 100644 src/app/dependencies.rs diff --git a/src/app/actor.rs b/src/app/actor.rs index 435b660..25351ca 100644 --- a/src/app/actor.rs +++ b/src/app/actor.rs @@ -6,16 +6,13 @@ //! [`InputEvent`](crate::input::event::InputEvent) from the channel registered //! with [`InputHandle`](crate::input::handle::InputHandle). //! -//! The actor stops when the input event channel closes (user quit) or when -//! initialization or I/O returns an unrecoverable error. Startup dependency -//! checks run inside [`AppActor::run`] before the first frame. +//! The actor stops when the input event channel closes (user quit) or when I/O +//! returns an unrecoverable error. Startup dependency checks run before this +//! actor is spawned. use std::ops::ControlFlow; -use tracing::{event, Level}; - use crate::{ app::{ - errors::AppError, flows::{ bookmarked::handle_bookmarked_patchsets, details_actions::handle_patchset_details, edit_config::handle_edit_config, latest::handle_latest_patchsets, @@ -27,7 +24,6 @@ use crate::{ App, }, input::{event::InputEvent, handle::InputHandle}, - render_prefs::PatchRenderer, terminal::{handle::TerminalHandle, messages::TerminalFrame}, ui::handle::UiHandle, }; @@ -68,68 +64,8 @@ impl AppActor { AppHandle::new(tokio::spawn(actor.run())) } - /// Verifies required and optional external binaries. - /// - /// A missing `b4` is a hard failure; all other missing binaries only emit - /// warnings. This replicates the former `check_external_deps` free function - /// that lived in `main.rs`. - fn initialize(&self) -> Result<(), AppError> { - let env = &*self.app.services.env; - let config = &self.app.state.config; - - if !env.which("b4") { - event!( - Level::ERROR, - "b4 is not installed, patchsets cannot be downloaded" - ); - return Err(AppError::Dependencies( - "b4 is not installed; patchsets cannot be downloaded".to_string(), - )); - } - - if !env.which("git") { - event!(Level::WARN, "git is not installed, send-email won't work"); - } - - match config.patch_renderer() { - PatchRenderer::Bat => { - if !env.which("bat") { - event!( - Level::WARN, - "bat is not installed, patch rendering will fallback to default" - ); - } - } - PatchRenderer::Delta => { - if !env.which("delta") { - event!( - Level::WARN, - "delta is not installed, patch rendering will fallback to default", - ); - } - } - PatchRenderer::DiffSoFancy => { - if !env.which("diff-so-fancy") { - event!( - Level::WARN, - "diff-so-fancy is not installed, patch rendering will fallback to default", - ); - } - } - _ => {} - } - - Ok(()) - } - async fn run(mut self) -> color_eyre::Result<()> { tracing::info!("app actor started"); - - let init_result = self.initialize(); - if let Err(ref e) = init_result { - tracing::warn!(error = %e, "app actor initialization failed"); - } - init_result?; tracing::info!("app actor initialized"); let mut loading = TerminalLoadingIndicator::new(self.terminal_handle.clone()); @@ -219,7 +155,6 @@ mod tests { use crate::{ app::{ - errors::AppError, screens::{ bookmarked::BookmarkedPatchsetsState, mail_list::MailingListSelectionState, CurrentScreen, @@ -426,35 +361,4 @@ mod tests { render.shutdown().await; } - #[tokio::test(flavor = "multi_thread")] - async fn missing_b4_causes_actor_to_return_dependencies_error() { - let mut env = MockEnvTrait::new(); - env.expect_which() - .withf(|name| name == "b4") - .returning(|_| false); - - let mut session = MockTerminalSessionApi::new(); - session.expect_draw().returning(|_| Ok(())); - - let terminal_handle = TerminalActor::spawn(Box::new(session)); - let ui_handle = UiActor::spawn(); - - 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( - minimal_app_with_env(env), - terminal_handle, - ui_handle, - input_handle, - event_rx, - ); - - let result = handle.run_until_done().await; - let err = result.unwrap_err(); - assert!(err - .downcast_ref::() - .is_some_and(|e| matches!(e, AppError::Dependencies(_)))); - } } diff --git a/src/app/dependencies.rs b/src/app/dependencies.rs new file mode 100644 index 0000000..e9ff811 --- /dev/null +++ b/src/app/dependencies.rs @@ -0,0 +1,123 @@ +use tracing::{event, Level}; + +use crate::{ + app::errors::AppError, + config::ConfigSnapshot, + infrastructure::env::EnvTrait, + render_prefs::PatchRenderer, +}; + +/// Verifies required and optional external binaries before the terminal starts. +/// +/// A missing `b4` is a hard failure; all other missing binaries only emit +/// warnings. This keeps fatal startup failures out of terminal raw mode. +pub(crate) fn check_external_deps( + env: &dyn EnvTrait, + config: &ConfigSnapshot, +) -> Result<(), AppError> { + if !env.which("b4") { + event!( + Level::ERROR, + "b4 is not installed, patchsets cannot be downloaded" + ); + return Err(AppError::Dependencies( + "b4 is not installed; patchsets cannot be downloaded".to_string(), + )); + } + + if !env.which("git") { + event!(Level::WARN, "git is not installed, send-email won't work"); + } + + match config.patch_renderer() { + PatchRenderer::Bat => { + if !env.which("bat") { + event!( + Level::WARN, + "bat is not installed, patch rendering will fallback to default" + ); + } + } + PatchRenderer::Delta => { + if !env.which("delta") { + event!( + Level::WARN, + "delta is not installed, patch rendering will fallback to default", + ); + } + } + PatchRenderer::DiffSoFancy => { + if !env.which("diff-so-fancy") { + event!( + Level::WARN, + "diff-so-fancy is not installed, patch rendering will fallback to default", + ); + } + } + _ => {} + } + + Ok(()) +} + +#[cfg(test)] +mod tests { + use crate::{ + config::{ConfigState, ValidatedConfigUpdate}, + infrastructure::env::MockEnvTrait, + render_prefs::PatchRenderer, + }; + + use super::*; + + #[test] + fn missing_b4_returns_dependencies_error() { + let mut env = MockEnvTrait::new(); + env.expect_which() + .withf(|name| name == "b4") + .returning(|_| false); + + let err = check_external_deps(&env, &ConfigState::default().to_snapshot()).unwrap_err(); + + assert!(matches!(err, AppError::Dependencies(_))); + } + + #[test] + fn missing_git_is_not_fatal() { + let mut env = MockEnvTrait::new(); + env.expect_which() + .withf(|name| name == "b4") + .returning(|_| true); + env.expect_which() + .withf(|name| name == "git") + .returning(|_| false); + + let result = check_external_deps(&env, &ConfigState::default().to_snapshot()); + + assert!(result.is_ok()); + } + + #[test] + fn missing_configured_renderer_is_not_fatal() { + let mut state = ConfigState::default(); + state.apply_update(&ValidatedConfigUpdate { + patch_renderer: Some(PatchRenderer::Bat), + ..Default::default() + }); + + let mut env = MockEnvTrait::new(); + env.expect_which() + .withf(|name| name == "b4") + .returning(|_| true); + env.expect_which() + .withf(|name| name == "git") + .returning(|_| true); + env.expect_which() + .withf(|name| name == "bat") + .returning(|_| false); + + let result = check_external_deps(&env, &state.to_snapshot()); + + assert!(result.is_ok()); + } +} diff --git a/src/app/mod.rs b/src/app/mod.rs index 0e87528..a527ee5 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -10,6 +10,7 @@ //! presentation data crosses the UI boundary only through [`AppViewModel`] via //! [`App::present`]. pub mod actor; +pub(crate) mod dependencies; pub mod errors; pub(crate) mod flows; pub mod handle; diff --git a/src/main.rs b/src/main.rs index c7ea6d4..7485f3b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -10,7 +10,7 @@ mod render_prefs; mod terminal; mod ui; -use app::{actor::AppActor, App}; +use app::{actor::AppActor, dependencies::check_external_deps, App}; use clap::Parser; use cli::Cli; use color_eyre::eyre::eyre; @@ -70,6 +70,8 @@ async fn main() -> color_eyre::Result<()> { ControlFlow::Continue(()) => {} } + check_external_deps(&env, &config)?; + let config_handle = ConfigActor::spawn(config_state, config_repo); let terminal_handle = TerminalActor::spawn(Box::new(CrosstermTerminalSession::new(init()?))); let ui_handle = UiActor::spawn(); From d6bc44f6be0ee52ede7f7518478a6f62f1dddb6d Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 9 Jul 2026 14:59:27 -0300 Subject: [PATCH 05/18] refactor(app): use rendered preview height for scroll bounds This commit bases patchset preview scroll bounds on the same rendered text projection used by the details view model instead of raw string line counts. Scroll-down and go-to-last-line behavior now match what the UI actually displays, without changing the short-preview saturating behavior. This commit is part of the architecture's refactoring phase 15. Signed-off-by: lorenzoberts --- src/app/actor.rs | 18 +----- src/app/dependencies.rs | 4 +- src/app/flows/open_patchset.rs | 11 +--- src/app/mod.rs | 4 -- src/app/screens/details_actions.rs | 89 ++++++++++++++++++++++++++++-- src/main.rs | 1 - 6 files changed, 91 insertions(+), 36 deletions(-) diff --git a/src/app/actor.rs b/src/app/actor.rs index 25351ca..8d4409e 100644 --- a/src/app/actor.rs +++ b/src/app/actor.rs @@ -163,9 +163,7 @@ mod tests { AppServices, }, config::{ConfigHandle, ConfigState}, - infrastructure::{ - env::MockEnvTrait, file_system::MockFileSystemTrait, shell::MockShellTrait, - }, + infrastructure::{file_system::MockFileSystemTrait, shell::MockShellTrait}, input::{event::InputEvent, handle::InputHandle, messages::InputMessage}, lore::{ application::{ @@ -193,7 +191,7 @@ mod tests { ConfigHandle::new(config_tx) } - fn minimal_app_with_env(env: MockEnvTrait) -> App { + fn minimal_app() -> App { let (lore_tx, _lore_rx) = mpsc::channel(1); let (render_tx, _render_rx) = mpsc::channel(1); @@ -230,18 +228,11 @@ mod tests { render: RenderHandle::new(render_tx), shell: Box::new(MockShellTrait::new()), fs: Box::new(MockFileSystemTrait::new()), - env: Box::new(env), config: dummy_config_handle(), }, } } - fn minimal_app() -> App { - let mut env = MockEnvTrait::new(); - env.expect_which().returning(|_| true); - minimal_app_with_env(env) - } - #[tokio::test(flavor = "multi_thread")] async fn input_channel_close_stops_actor_and_returns_ok() { let mut session = MockTerminalSessionApi::new(); @@ -322,9 +313,6 @@ mod tests { .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() @@ -340,7 +328,6 @@ mod tests { bootstrap, Box::new(MockFileSystemTrait::new()), Box::new(MockShellTrait::new()), - Box::new(env), lore_api.clone(), render.clone(), ) @@ -360,5 +347,4 @@ mod tests { lore_api.shutdown().await; render.shutdown().await; } - } diff --git a/src/app/dependencies.rs b/src/app/dependencies.rs index e9ff811..3a074fc 100644 --- a/src/app/dependencies.rs +++ b/src/app/dependencies.rs @@ -1,9 +1,7 @@ use tracing::{event, Level}; use crate::{ - app::errors::AppError, - config::ConfigSnapshot, - infrastructure::env::EnvTrait, + app::errors::AppError, config::ConfigSnapshot, infrastructure::env::EnvTrait, render_prefs::PatchRenderer, }; diff --git a/src/app/flows/open_patchset.rs b/src/app/flows/open_patchset.rs index fc4d313..5896eac 100644 --- a/src/app/flows/open_patchset.rs +++ b/src/app/flows/open_patchset.rs @@ -3,10 +3,7 @@ use crate::app::{popup::AppPopup, screens::CurrentScreen, App, B4Result}; #[derive(Debug, PartialEq)] enum OpenPatchsetAction { ShowDetails, - ShowError { - origin: CurrentScreen, - body: String, - }, + ShowError { origin: CurrentScreen, body: String }, } pub(super) fn apply_open_patchset_result( @@ -52,10 +49,8 @@ mod tests { #[test] fn patch_found_opens_details() { - let action = resolve_open_patchset_result( - CurrentScreen::LatestPatchsets, - Ok(B4Result::PatchFound), - ); + let action = + resolve_open_patchset_result(CurrentScreen::LatestPatchsets, Ok(B4Result::PatchFound)); assert_eq!(OpenPatchsetAction::ShowDetails, action); } diff --git a/src/app/mod.rs b/src/app/mod.rs index a527ee5..b45bfdb 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -30,7 +30,6 @@ use std::path::PathBuf; use crate::{ config::{ConfigHandle, ConfigSnapshot}, infrastructure::{ - env::EnvTrait, file_system::FileSystemTrait, monitoring::logging::garbage_collector::collect_garbage, shell::{ShellCommand, ShellTrait}, @@ -62,7 +61,6 @@ pub struct AppServices { pub render: RenderHandle, pub shell: Box, pub fs: Box, - pub env: Box, pub config: ConfigHandle, } @@ -93,7 +91,6 @@ impl App { bootstrap: BootstrapLoreData, fs: Box, shell: Box, - env: Box, lore_api: LoreApiHandle, render: RenderHandle, ) -> color_eyre::Result { @@ -131,7 +128,6 @@ impl App { render, shell, fs, - env, config: config_handle, }, }) diff --git a/src/app/screens/details_actions.rs b/src/app/screens/details_actions.rs index 7218225..ae3c17a 100644 --- a/src/app/screens/details_actions.rs +++ b/src/app/screens/details_actions.rs @@ -1,6 +1,8 @@ use std::collections::{HashMap, HashSet}; use std::path::Path; +use ansi_to_tui::IntoText; + use crate::{ config::{ConfigSnapshot, KernelTree}, infrastructure::{ @@ -48,6 +50,10 @@ pub struct PatchsetDetailsState { const LAST_LINE_PADDING: usize = 10; +fn rendered_preview_height(preview: &str) -> usize { + preview.into_text().unwrap_or_default().height() +} + #[derive(Clone, Hash, Eq, PartialEq)] pub enum PatchsetAction { Bookmark, @@ -123,8 +129,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].lines().count(); + let number_of_lines = rendered_preview_height(&self.patches_preview[self.preview_index]); if (self.preview_scroll_offset + n) <= number_of_lines { self.preview_scroll_offset += n; } @@ -137,8 +142,7 @@ 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].lines().count(); + let number_of_lines = rendered_preview_height(&self.patches_preview[self.preview_index]); self.preview_scroll_offset = number_of_lines.saturating_sub(LAST_LINE_PADDING); } @@ -473,3 +477,80 @@ impl PatchsetDetailsState { } } } + +#[cfg(test)] +mod tests { + use std::collections::{HashMap, HashSet}; + + use serde_xml_rs::from_str; + + use super::*; + + fn test_patch() -> Patch { + from_str( + r#" + + + Foo Bar + foo@bar.foo.bar + + [PATCH 1/1] test patch + 2024-07-06T19:15:48Z + + urn:uuid:123-abcd-1f2a3b + + + "#, + ) + .expect("test patch XML should deserialize") + } + + fn details_state_with_preview(preview: &str) -> PatchsetDetailsState { + PatchsetDetailsState { + representative_patch: test_patch(), + raw_patches: vec!["raw patch".to_string()], + patches_preview: vec![preview.to_string()], + has_cover_letter: false, + patches_to_reply: vec![false], + patchset_path: "/tmp/patchset.mbx".to_string(), + preview_index: 0, + preview_scroll_offset: 0, + preview_pan: 0, + preview_fullscreen: false, + patchset_actions: HashMap::from([ + (PatchsetAction::Bookmark, false), + (PatchsetAction::ReplyWithReviewedBy, false), + (PatchsetAction::Apply, false), + ]), + reviewed_by: vec![HashSet::new()], + tested_by: vec![HashSet::new()], + acked_by: vec![HashSet::new()], + last_screen: CurrentScreen::LatestPatchsets, + } + } + + #[test] + fn rendered_height_accounts_for_rendered_text_projection() { + let preview = "\u{1b}[32mrendered line\u{1b}[0m\nsecond line"; + + assert_eq!(2, rendered_preview_height(preview)); + } + + #[test] + fn preview_scroll_down_uses_rendered_height() { + let mut state = details_state_with_preview("\u{1b}[32mrendered line\u{1b}[0m\nsecond line"); + + state.preview_scroll_down(2); + + assert_eq!(2, state.preview_scroll_offset); + } + + #[test] + fn go_to_last_line_saturates_for_short_rendered_preview() { + let mut state = details_state_with_preview("short preview"); + + state.go_to_last_line(); + + assert_eq!(0, state.preview_scroll_offset); + } +} diff --git a/src/main.rs b/src/main.rs index 7485f3b..c6231ba 100644 --- a/src/main.rs +++ b/src/main.rs @@ -123,7 +123,6 @@ async fn main() -> color_eyre::Result<()> { bootstrap, Box::new(OsFileSystem), Box::new(OsShell), - Box::new(env), lore_api.clone(), render.clone(), )?; From 080ef1c05d4ad2437c05dcbb2786c9c02978b21f Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 9 Jul 2026 15:19:01 -0300 Subject: [PATCH 06/18] style: move inline full paths into imports This commit replaces fully qualified std, tokio, ratatui, and other common paths with module-level imports across app, UI, terminal, render, config, and infrastructure code. The change is formatting-only and does not alter runtime behavior. This commit is part of the architecture's refactoring phase 15. Signed-off-by: lorenzoberts --- src/app/actor.rs | 15 ++++++---- src/app/flows/bookmarked.rs | 4 ++- src/app/flows/details_actions.rs | 5 ++-- src/app/flows/edit_config.rs | 3 +- src/app/flows/latest.rs | 4 ++- src/app/flows/mail_list.rs | 7 +++-- src/app/flows/open_patchset.rs | 6 ++-- src/app/handle.rs | 7 +++-- src/app/loading.rs | 28 +++++++++++-------- src/app/mod.rs | 25 +++++++++-------- src/app/popup.rs | 28 +++++++++++-------- src/app/screens/details_actions.rs | 3 +- src/app/screens/edit_config.rs | 14 ++++++---- src/app/screens/latest.rs | 4 +-- src/app/screens/mail_list.rs | 4 +-- src/app/updates.rs | 4 ++- src/cli.rs | 7 +++-- src/config/actor.rs | 7 +++-- src/config/repository.rs | 5 ++-- src/config/service.rs | 4 ++- src/config/state.rs | 7 +++-- src/infrastructure/errors.rs | 21 ++++++++------ src/infrastructure/file_system/trait.rs | 4 +-- .../monitoring/logging/garbage_collector.rs | 8 ++++-- src/infrastructure/monitoring/logging/mod.rs | 6 ++-- .../logging/multi_log_file_writer.rs | 16 ++++++----- src/infrastructure/terminal.rs | 5 ++-- src/input/actor.rs | 8 +++--- src/lore/application/actor.rs | 3 +- src/lore/domain/mailing_list.rs | 6 ++-- src/lore/domain/patch.rs | 4 +-- src/lore/infrastructure/persistence.rs | 10 ++++--- src/main.rs | 12 ++++---- src/render/actor.rs | 3 +- src/render/cover_renderer.rs | 10 ++----- src/render/patch_renderer.rs | 10 +++---- src/render_prefs.rs | 7 +++-- src/terminal/actor.rs | 3 +- src/terminal/errors.rs | 4 ++- src/terminal/session.rs | 4 +-- src/ui/actor.rs | 11 +++++--- src/ui/screens/bookmarked.rs | 4 +-- src/ui/screens/details.rs | 8 +++--- src/ui/screens/latest.rs | 4 +-- src/ui/screens/mailing_list.rs | 4 +-- src/ui/screens/popup.rs | 16 ++++------- 46 files changed, 217 insertions(+), 165 deletions(-) diff --git a/src/app/actor.rs b/src/app/actor.rs index 8d4409e..3e44bb9 100644 --- a/src/app/actor.rs +++ b/src/app/actor.rs @@ -11,6 +11,9 @@ //! actor is spawned. use std::ops::ControlFlow; +use color_eyre::{eyre::eyre, Result}; +use tokio::{spawn, sync::mpsc}; + use crate::{ app::{ flows::{ @@ -40,7 +43,7 @@ pub struct AppActor { terminal_handle: TerminalHandle, ui_handle: UiHandle, input_handle: InputHandle, - event_rx: tokio::sync::mpsc::Receiver, + event_rx: mpsc::Receiver, } impl AppActor { @@ -51,7 +54,7 @@ impl AppActor { terminal_handle: TerminalHandle, ui_handle: UiHandle, input_handle: InputHandle, - event_rx: tokio::sync::mpsc::Receiver, + event_rx: mpsc::Receiver, ) -> AppHandle { tracing::debug!("spawning app actor"); let actor = Self { @@ -61,10 +64,10 @@ impl AppActor { input_handle, event_rx, }; - AppHandle::new(tokio::spawn(actor.run())) + AppHandle::new(spawn(actor.run())) } - async fn run(mut self) -> color_eyre::Result<()> { + async fn run(mut self) -> Result<()> { tracing::info!("app actor started"); tracing::info!("app actor initialized"); @@ -77,7 +80,7 @@ impl AppActor { .ui_handle .build_scene(self.app.present()) .await - .map_err(|e| color_eyre::eyre::eyre!("{e}"))?; + .map_err(|e| eyre!("{e}"))?; self.terminal_handle .draw(TerminalFrame::Main(Box::new(scene))) .await @@ -114,7 +117,7 @@ async fn on_input( input: InputEvent, terminal_handle: &TerminalHandle, loading: &mut TerminalLoadingIndicator, -) -> color_eyre::Result> { +) -> Result> { if let Some(popup) = app.state.popup.as_mut() { if input == InputEvent::ClosePopup { app.state.popup = None; diff --git a/src/app/flows/bookmarked.rs b/src/app/flows/bookmarked.rs index 1937af1..e804ae5 100644 --- a/src/app/flows/bookmarked.rs +++ b/src/app/flows/bookmarked.rs @@ -1,5 +1,7 @@ use tracing::debug; +use color_eyre::Result; + use crate::{ app::{loading::LoadingIndicator, popup::AppPopup, screens::CurrentScreen, App}, input::event::InputEvent, @@ -11,7 +13,7 @@ pub async fn handle_bookmarked_patchsets( app: &mut App, input: InputEvent, loading: &mut dyn LoadingIndicator, -) -> color_eyre::Result<()> { +) -> Result<()> { match input { InputEvent::OpenHelp => { let popup = generate_help_popup(); diff --git a/src/app/flows/details_actions.rs b/src/app/flows/details_actions.rs index 28b18ae..15a101c 100644 --- a/src/app/flows/details_actions.rs +++ b/src/app/flows/details_actions.rs @@ -1,5 +1,6 @@ use std::time::Duration; +use color_eyre::Result; use ratatui::crossterm::event::KeyCode; use tracing::debug; @@ -15,7 +16,7 @@ pub async fn handle_patchset_details( app: &mut App, input: InputEvent, terminal_handle: &TerminalHandle, -) -> color_eyre::Result<()> { +) -> Result<()> { let patchset_details_and_actions = app .state .lore @@ -108,7 +109,7 @@ pub async fn handle_patchset_details( async fn preview_scroll_lines( amount: ScrollAmount, terminal_handle: &TerminalHandle, -) -> color_eyre::Result { +) -> Result { let (_, height) = terminal_handle.size().await?; Ok(match amount { ScrollAmount::Line => 1, diff --git a/src/app/flows/edit_config.rs b/src/app/flows/edit_config.rs index 44b2157..73a04b7 100644 --- a/src/app/flows/edit_config.rs +++ b/src/app/flows/edit_config.rs @@ -1,3 +1,4 @@ +use color_eyre::Result; use tracing::debug; use crate::{ @@ -5,7 +6,7 @@ use crate::{ input::event::InputEvent, }; -pub async fn handle_edit_config(app: &mut App, input: InputEvent) -> color_eyre::Result<()> { +pub async fn handle_edit_config(app: &mut App, input: InputEvent) -> Result<()> { let Some(is_editing) = app .state .config_state diff --git a/src/app/flows/latest.rs b/src/app/flows/latest.rs index 9ef3051..3953585 100644 --- a/src/app/flows/latest.rs +++ b/src/app/flows/latest.rs @@ -1,5 +1,7 @@ use tracing::debug; +use color_eyre::Result; + use crate::{ app::{loading::LoadingIndicator, popup::AppPopup, screens::CurrentScreen, App}, input::event::InputEvent, @@ -11,7 +13,7 @@ pub async fn handle_latest_patchsets( app: &mut App, input: InputEvent, loading: &mut dyn LoadingIndicator, -) -> color_eyre::Result<()> { +) -> Result<()> { match input { InputEvent::OpenHelp => { let popup = generate_help_popup(); diff --git a/src/app/flows/mail_list.rs b/src/app/flows/mail_list.rs index bea7031..82ac011 100644 --- a/src/app/flows/mail_list.rs +++ b/src/app/flows/mail_list.rs @@ -1,17 +1,18 @@ use std::ops::ControlFlow; +use color_eyre::Result; +use tracing::debug; + use crate::{ app::{loading::LoadingIndicator, popup::AppPopup, screens::CurrentScreen, App}, input::event::InputEvent, }; -use tracing::debug; - pub async fn handle_mailing_list_selection( app: &mut App, input: InputEvent, loading: &mut dyn LoadingIndicator, -) -> color_eyre::Result> { +) -> Result> { match input { InputEvent::OpenHelp => { let popup = generate_help_popup(); diff --git a/src/app/flows/open_patchset.rs b/src/app/flows/open_patchset.rs index 5896eac..3431254 100644 --- a/src/app/flows/open_patchset.rs +++ b/src/app/flows/open_patchset.rs @@ -1,3 +1,5 @@ +use color_eyre::Result; + use crate::app::{popup::AppPopup, screens::CurrentScreen, App, B4Result}; #[derive(Debug, PartialEq)] @@ -9,7 +11,7 @@ enum OpenPatchsetAction { pub(super) fn apply_open_patchset_result( app: &mut App, origin: CurrentScreen, - result: color_eyre::Result, + result: Result, ) { match resolve_open_patchset_result(origin, result) { OpenPatchsetAction::ShowDetails => { @@ -24,7 +26,7 @@ pub(super) fn apply_open_patchset_result( fn resolve_open_patchset_result( origin: CurrentScreen, - result: color_eyre::Result, + result: Result, ) -> OpenPatchsetAction { match result { Ok(B4Result::PatchFound) => OpenPatchsetAction::ShowDetails, diff --git a/src/app/handle.rs b/src/app/handle.rs index c4e9777..3a9fd76 100644 --- a/src/app/handle.rs +++ b/src/app/handle.rs @@ -1,3 +1,4 @@ +use color_eyre::Result; use tokio::task::JoinHandle; /// Handle to the `AppActor` task. @@ -6,17 +7,17 @@ use tokio::task::JoinHandle; /// 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>, + join: JoinHandle>, } impl AppHandle { - pub(crate) fn new(join: JoinHandle>) -> Self { + pub(crate) fn new(join: JoinHandle>) -> Self { Self { join } } /// Blocks the caller until the actor task completes, propagating any error /// returned by the actor's run loop. - pub async fn run_until_done(self) -> color_eyre::Result<()> { + pub async fn run_until_done(self) -> Result<()> { self.join.await? } } diff --git a/src/app/loading.rs b/src/app/loading.rs index 1f60a70..d1fdc32 100644 --- a/src/app/loading.rs +++ b/src/app/loading.rs @@ -3,10 +3,16 @@ use std::{ atomic::{AtomicBool, Ordering}, Arc, }, + thread, time::Duration, }; -use tokio::task::JoinHandle; +use color_eyre::{eyre::eyre, Report, Result}; +use tokio::{ + runtime::Handle, + spawn, + task::{block_in_place, JoinHandle}, +}; use crate::terminal::{handle::TerminalHandle, messages::TerminalFrame, TerminalError}; @@ -14,7 +20,7 @@ pub(crate) const LOADING_FRAME_INTERVAL: Duration = Duration::from_millis(200); pub(crate) trait LoadingIndicator: Send { fn start(&mut self, title: String); - fn stop(&mut self) -> color_eyre::Result<()>; + fn stop(&mut self) -> Result<()>; } pub(crate) struct TerminalLoadingIndicator { @@ -44,7 +50,7 @@ impl LoadingIndicator for TerminalLoadingIndicator { let terminal_handle = self.terminal_handle.clone(); self.running = Some(running); - self.spinner_task = Some(tokio::spawn(async move { + self.spinner_task = Some(spawn(async move { while running_clone.load(Ordering::Relaxed) { if terminal_handle .draw(TerminalFrame::Loading(title.clone())) @@ -54,14 +60,14 @@ impl LoadingIndicator for TerminalLoadingIndicator { break; } - std::thread::sleep(LOADING_FRAME_INTERVAL); + thread::sleep(LOADING_FRAME_INTERVAL); } })); - std::thread::sleep(LOADING_FRAME_INTERVAL); + thread::sleep(LOADING_FRAME_INTERVAL); } - fn stop(&mut self) -> color_eyre::Result<()> { + fn stop(&mut self) -> Result<()> { let Some(spinner_task) = self.spinner_task.take() else { return Ok(()); }; @@ -70,16 +76,16 @@ impl LoadingIndicator for TerminalLoadingIndicator { running.store(false, Ordering::Relaxed); } - tokio::task::block_in_place(|| { - tokio::runtime::Handle::current().block_on(async { spinner_task.await.ok() }); + block_in_place(|| { + Handle::current().block_on(async { spinner_task.await.ok() }); }); Ok(()) } } -pub(crate) fn terminal_error(error: TerminalError) -> color_eyre::Report { - color_eyre::eyre::eyre!("{error}") +pub(crate) fn terminal_error(error: TerminalError) -> Report { + eyre!("{error}") } #[cfg(test)] @@ -102,7 +108,7 @@ mod tests { let mut loading = TerminalLoadingIndicator::new(handle); loading.start("Fetching mailing lists".to_string()); - std::thread::sleep(LOADING_FRAME_INTERVAL); + thread::sleep(LOADING_FRAME_INTERVAL); loading.stop().unwrap(); } } diff --git a/src/app/mod.rs b/src/app/mod.rs index b45bfdb..8ee2cfe 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -22,10 +22,13 @@ pub mod state; pub mod updates; pub mod view_model; -use color_eyre::eyre::{bail, eyre}; +use color_eyre::{ + eyre::{bail, eyre}, + Result, +}; use tracing::{debug, event, info, warn, Level}; -use std::path::PathBuf; +use std::{path::PathBuf, str}; use crate::{ config::{ConfigHandle, ConfigSnapshot}, @@ -93,7 +96,7 @@ impl App { shell: Box, lore_api: LoreApiHandle, render: RenderHandle, - ) -> color_eyre::Result { + ) -> Result { event!(Level::INFO, "patch-hub started"); collect_garbage(&config); @@ -160,7 +163,7 @@ impl App { } /// Fetches (or re-fetches) the current page of latest patchsets from Lore. - pub async fn fetch_latest_current_page(&mut self) -> color_eyre::Result<()> { + pub async fn fetch_latest_current_page(&mut self) -> Result<()> { let lore_api = &self.services.lore_api; let latest_patchsets = &mut self.state.lore.latest_patchsets; if let Some(patchsets) = latest_patchsets.as_mut() { @@ -181,7 +184,7 @@ impl App { } /// Refreshes available mailing lists and updates [`LoreUiState::mailing_list_selection`]. - pub async fn refresh_mailing_lists(&mut self) -> color_eyre::Result<()> { + pub async fn refresh_mailing_lists(&mut self) -> Result<()> { debug!("refreshing mailing lists"); let result = self .state @@ -197,7 +200,7 @@ impl App { } /// Loads patchset details into [`LoreUiState::details`]. - pub async fn open_patchset_details(&mut self) -> color_eyre::Result { + pub async fn open_patchset_details(&mut self) -> Result { let representative_patch: Patch; let mut is_patchset_bookmarked = true; @@ -288,7 +291,7 @@ impl App { /// # Panics /// /// Panics if [`LoreUiState::details`] is `None`. - pub async fn consolidate_patchset_actions(&mut self) -> color_eyre::Result<()> { + pub async fn consolidate_patchset_actions(&mut self) -> Result<()> { debug!("consolidating patchset actions"); self.sync_patchset_bookmark().await?; self.execute_reviewed_reply().await?; @@ -297,7 +300,7 @@ impl App { Ok(()) } - async fn sync_patchset_bookmark(&mut self) -> color_eyre::Result<()> { + async fn sync_patchset_bookmark(&mut self) -> Result<()> { let details = self .state .lore @@ -337,7 +340,7 @@ impl App { Ok(()) } - async fn execute_reviewed_reply(&mut self) -> color_eyre::Result<()> { + async fn execute_reviewed_reply(&mut self) -> Result<()> { let details = self .state .lore @@ -377,7 +380,7 @@ impl App { .shell .execute(&mktemp_cmd) .map_err(|e| eyre!("failed to create temp directory: {}", e))?; - let tmp_dir_str = std::str::from_utf8(&tmp_out.stdout) + let tmp_dir_str = str::from_utf8(&tmp_out.stdout) .map_err(|e| eyre!("invalid utf-8 in temp dir path: {}", e))? .trim() .to_string(); @@ -487,7 +490,7 @@ impl App { } /// Applies edited values from [`ConfigUiState::edit_config`] into [`AppState::config`]. - pub async fn consolidate_edit_config(&mut self) -> color_eyre::Result<()> { + pub async fn consolidate_edit_config(&mut self) -> 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(); diff --git a/src/app/popup.rs b/src/app/popup.rs index 139a393..c066a7a 100644 --- a/src/app/popup.rs +++ b/src/app/popup.rs @@ -4,7 +4,12 @@ //! Each variant owns all data required to present the popup and tracks scroll //! position so the presentation layer receives a read-only snapshot. -use crate::{app::screens::details_actions::PatchsetDetailsState, input::event::InputEvent}; +use std::collections::HashSet; + +use crate::{ + app::screens::details_actions::PatchsetDetailsState, input::event::InputEvent, + lore::domain::patch::Author, +}; // --------------------------------------------------------------------------- // Main enum @@ -76,18 +81,17 @@ impl AppPopup { let i = details.preview_index; let mut columns: usize = 0; - let mut format_section = - |authors: &std::collections::HashSet| -> String { - let mut text = String::new(); - for author in authors { - let line = format!(" - {author}\n"); - if line.len() > columns { - columns = line.len(); - } - text.push_str(&line); + let mut format_section = |authors: &HashSet| -> String { + let mut text = String::new(); + for author in authors { + let line = format!(" - {author}\n"); + if line.len() > columns { + columns = line.len(); } - text - }; + text.push_str(&line); + } + text + }; let reviewed_by = format_section(&details.reviewed_by[i]); let tested_by = format_section(&details.tested_by[i]); diff --git a/src/app/screens/details_actions.rs b/src/app/screens/details_actions.rs index ae3c17a..a7aa6fd 100644 --- a/src/app/screens/details_actions.rs +++ b/src/app/screens/details_actions.rs @@ -2,6 +2,7 @@ use std::collections::{HashMap, HashSet}; use std::path::Path; use ansi_to_tui::IntoText; +use chrono::Utc; use crate::{ config::{ConfigSnapshot, KernelTree}, @@ -390,7 +391,7 @@ impl PatchsetDetailsState { let target_branch_name = format!( "{}{}", config.git_am_branch_prefix(), - chrono::Utc::now().format("%Y-%m-%d-%H-%M-%S") + Utc::now().format("%Y-%m-%d-%H-%M-%S") ); let out = shell diff --git a/src/app/screens/edit_config.rs b/src/app/screens/edit_config.rs index a629cb7..ef1e825 100644 --- a/src/app/screens/edit_config.rs +++ b/src/app/screens/edit_config.rs @@ -1,7 +1,11 @@ -use color_eyre::eyre::bail; +use color_eyre::{eyre::bail, Report}; use derive_getters::Getters; -use std::{collections::HashMap, fmt::Display}; +use std::{ + collections::HashMap, + fmt::{self, Display, Formatter}, + mem, +}; use crate::config::{ConfigSnapshot, ConfigUpdateDraft}; @@ -109,7 +113,7 @@ impl EditConfigState { pub fn stage_edit(&mut self) { if let Ok(editable_config) = EditableConfig::try_from(self.highlighted) { self.config_buffer - .insert(editable_config, std::mem::take(&mut self.curr_edit)); + .insert(editable_config, mem::take(&mut self.curr_edit)); } } @@ -150,7 +154,7 @@ enum EditableConfig { } impl TryFrom for EditableConfig { - type Error = color_eyre::Report; + type Error = Report; fn try_from(value: usize) -> Result { match value { @@ -168,7 +172,7 @@ impl TryFrom for EditableConfig { } impl Display for EditableConfig { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { match self { EditableConfig::PageSize => write!(f, "Page Size"), EditableConfig::CacheDir => write!(f, "Cache Directory"), diff --git a/src/app/screens/latest.rs b/src/app/screens/latest.rs index 18e5dcb..0d59b24 100644 --- a/src/app/screens/latest.rs +++ b/src/app/screens/latest.rs @@ -1,4 +1,4 @@ -use color_eyre::eyre::bail; +use color_eyre::{eyre::bail, Result}; use crate::lore::{ application::{cache::CacheMode, errors::LoreError, handle::LoreApiHandle}, @@ -43,7 +43,7 @@ impl LatestPatchsetsState { &mut self, lore_api: &LoreApiHandle, mode: CacheMode, - ) -> color_eyre::Result<()> { + ) -> Result<()> { match lore_api .fetch_feed_page( self.target_list.clone(), diff --git a/src/app/screens/mail_list.rs b/src/app/screens/mail_list.rs index e6dff7d..8911930 100644 --- a/src/app/screens/mail_list.rs +++ b/src/app/screens/mail_list.rs @@ -1,4 +1,4 @@ -use color_eyre::eyre::bail; +use color_eyre::{eyre::bail, Result}; use crate::lore::{ application::{cache::CacheMode, handle::LoreApiHandle}, @@ -18,7 +18,7 @@ impl MailingListSelectionState { &mut self, lore_api: &LoreApiHandle, mode: CacheMode, - ) -> color_eyre::Result<()> { + ) -> Result<()> { match lore_api.fetch_available_lists(mode).await { Ok(available_mailing_lists) => { self.mailing_lists = available_mailing_lists; diff --git a/src/app/updates.rs b/src/app/updates.rs index f58c722..826ee22 100644 --- a/src/app/updates.rs +++ b/src/app/updates.rs @@ -1,3 +1,5 @@ +use color_eyre::Result; + use crate::app::{loading::LoadingIndicator, screens::CurrentScreen, App}; impl App { @@ -5,7 +7,7 @@ impl App { pub async fn process_system_updates( &mut self, loading: &mut (dyn LoadingIndicator + Send), - ) -> color_eyre::Result<()> { + ) -> Result<()> { match self.state.navigation.current_screen { CurrentScreen::MailingListSelection => { if self diff --git a/src/cli.rs b/src/cli.rs index 59515f7..7ff96b2 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -1,5 +1,6 @@ use clap::Parser; -use color_eyre::eyre::eyre; +use color_eyre::{eyre::eyre, Result}; +use serde_json::to_string_pretty; use std::ops::ControlFlow; @@ -17,9 +18,9 @@ impl Cli { /// Resolves command line arguments that may finish before the TUI starts. /// /// Some arguments may finish the program early (returning `ControlFlow::Break`) - pub fn resolve(&self, config: &ConfigSnapshot) -> ControlFlow, ()> { + pub fn resolve(&self, config: &ConfigSnapshot) -> ControlFlow, ()> { if self.show_configs { - match serde_json::to_string_pretty(&config) { + match to_string_pretty(&config) { Err(err) => return ControlFlow::Break(Err(eyre!(err))), Ok(config) => println!("patch-hub configurations:\n{config}"), } diff --git a/src/config/actor.rs b/src/config/actor.rs index a7f56b7..ccb2b50 100644 --- a/src/config/actor.rs +++ b/src/config/actor.rs @@ -5,7 +5,10 @@ //! and persists successful updates through [`JsonConfigRepository`](crate::config::JsonConfigRepository). use std::ops::ControlFlow; -use tokio::sync::{mpsc, oneshot}; +use tokio::{ + spawn, + sync::{mpsc, oneshot}, +}; use crate::{ config::{ @@ -45,7 +48,7 @@ where channel_size = DEFAULT_CONFIG_CHANNEL_SIZE, "spawning config actor" ); - tokio::spawn(Self::new(state, repo, rx).run()); + spawn(Self::new(state, repo, rx).run()); ConfigHandle::new(tx) } diff --git a/src/config/repository.rs b/src/config/repository.rs index 5be91d5..67c9a37 100644 --- a/src/config/repository.rs +++ b/src/config/repository.rs @@ -1,5 +1,7 @@ use std::path::Path; +use serde_json::to_writer_pretty; + use crate::config::errors::ConfigError; use crate::config::state::ConfigState; use crate::config::DEFAULT_CONFIG_PATH_SUFFIX; @@ -57,8 +59,7 @@ impl ConfigRepository for JsonConfigRepository { .fs .create_writer(Path::new(&tmp_filename)) .map_err(ConfigError::from)?; - serde_json::to_writer_pretty(tmp_file, state) - .map_err(|e| ConfigError::Save(e.to_string()))?; + to_writer_pretty(tmp_file, state).map_err(|e| ConfigError::Save(e.to_string()))?; } self.fs .rename(Path::new(&tmp_filename), config_path) diff --git a/src/config/service.rs b/src/config/service.rs index 5a77420..d1e54ee 100644 --- a/src/config/service.rs +++ b/src/config/service.rs @@ -1,5 +1,7 @@ use std::path::Path; +use serde_json::from_str; + use crate::config::env_overrides; use crate::config::errors::ConfigError; use crate::config::parsing::{parse_cover_renderer, parse_patch_renderer}; @@ -32,7 +34,7 @@ fn load_initial_state( let fs = repo.fs(); if fs.is_file(Path::new(path)) { match fs.read_to_string(Path::new(path)) { - Ok(file_contents) => match serde_json::from_str(&file_contents) { + Ok(file_contents) => match from_str(&file_contents) { Ok(config) => return config, Err(e) => eprintln!("Failed to parse config file {path}: {e}"), }, diff --git a/src/config/state.rs b/src/config/state.rs index a71bbb2..963d965 100644 --- a/src/config/state.rs +++ b/src/config/state.rs @@ -1,7 +1,10 @@ use derive_getters::Getters; use patch_hub_proc_macros::serde_individual_default; use serde::Serialize; -use std::collections::{HashMap, HashSet}; +use std::{ + collections::{HashMap, HashSet}, + env, +}; use crate::config::update::ValidatedConfigUpdate; use crate::infrastructure::env::EnvTrait; @@ -39,7 +42,7 @@ pub struct ConfigState { impl Default for ConfigState { fn default() -> Self { - let home = std::env::var("HOME").unwrap_or_else(|_| { + let home = env::var("HOME").unwrap_or_else(|_| { eprintln!("$HOME environment variable not set, using current directory"); ".".to_string() }); diff --git a/src/infrastructure/errors.rs b/src/infrastructure/errors.rs index d07356d..452e926 100644 --- a/src/infrastructure/errors.rs +++ b/src/infrastructure/errors.rs @@ -1,4 +1,9 @@ -use std::panic; +use std::{error::Error, panic}; + +use color_eyre::{ + config::HookBuilder, + eyre::{set_hook, Result}, +}; use super::terminal::restore; @@ -8,8 +13,8 @@ use super::terminal::restore; /// Normal application shutdown restores the terminal through /// [`crate::terminal::handle::TerminalHandle::shutdown`]. These hooks keep a /// direct [`super::terminal::restore`] fallback for panics and fatal errors. -pub fn install_hooks() -> color_eyre::Result<()> { - let (panic_hook, eyre_hook) = color_eyre::config::HookBuilder::default().into_hooks(); +pub fn install_hooks() -> Result<()> { + let (panic_hook, eyre_hook) = HookBuilder::default().into_hooks(); // convert from a color_eyre PanicHook to a standard panic hook let panic_hook = panic_hook.into_panic_hook(); @@ -20,12 +25,10 @@ pub fn install_hooks() -> color_eyre::Result<()> { // convert from a color_eyre EyreHook to a eyre ErrorHook let eyre_hook = eyre_hook.into_eyre_hook(); - color_eyre::eyre::set_hook(Box::new( - move |error: &(dyn std::error::Error + 'static)| { - restore().unwrap(); - eyre_hook(error) - }, - ))?; + set_hook(Box::new(move |error: &(dyn Error + 'static)| { + restore().unwrap(); + eyre_hook(error) + }))?; Ok(()) } diff --git a/src/infrastructure/file_system/trait.rs b/src/infrastructure/file_system/trait.rs index 3782b0e..c1a1fb5 100644 --- a/src/infrastructure/file_system/trait.rs +++ b/src/infrastructure/file_system/trait.rs @@ -1,7 +1,7 @@ use mockall::automock; use thiserror::Error; -use std::{io, path::Path}; +use std::{fs::Metadata, io, path::Path}; #[derive(Debug, Error)] pub enum FileSystemError { @@ -20,5 +20,5 @@ pub trait FileSystemTrait: Send + Sync { fn rename(&self, from: &Path, to: &Path) -> Result<(), FileSystemError>; fn create_writer(&self, path: &Path) -> Result, FileSystemError>; fn open_bufreader(&self, path: &Path) -> Result, FileSystemError>; - fn metadata(&self, path: &Path) -> Result; + fn metadata(&self, path: &Path) -> Result; } diff --git a/src/infrastructure/monitoring/logging/garbage_collector.rs b/src/infrastructure/monitoring/logging/garbage_collector.rs index 41ab09c..48f3845 100644 --- a/src/infrastructure/monitoring/logging/garbage_collector.rs +++ b/src/infrastructure/monitoring/logging/garbage_collector.rs @@ -2,6 +2,8 @@ //! //! This module is responsible for cleaning up the log files. +use std::{fs, time::SystemTime}; + use tracing::{event, Level}; use crate::config::ConfigSnapshot; @@ -13,9 +15,9 @@ pub fn collect_garbage(config: &ConfigSnapshot) { return; } - let now = std::time::SystemTime::now(); + let now = SystemTime::now(); let logs_path = config.logs_path(); - let Ok(logs) = std::fs::read_dir(logs_path) else { + let Ok(logs) = fs::read_dir(logs_path) else { event!( Level::ERROR, "Failed to read the logs directory during garbage collection" @@ -43,7 +45,7 @@ pub fn collect_garbage(config: &ConfigSnapshot) { }; let age = age.as_secs() / 60 / 60 / 24; - if age as usize > config.max_log_age() && std::fs::remove_file(log.path()).is_err() { + if age as usize > config.max_log_age() && fs::remove_file(log.path()).is_err() { event!( Level::WARN, "Failed to remove the log file: {}", diff --git a/src/infrastructure/monitoring/logging/mod.rs b/src/infrastructure/monitoring/logging/mod.rs index bd5fb04..312c593 100644 --- a/src/infrastructure/monitoring/logging/mod.rs +++ b/src/infrastructure/monitoring/logging/mod.rs @@ -3,6 +3,7 @@ use std::{ sync::{Arc, Mutex}, }; +use chrono::Local; use multi_log_file_writer::{create_non_blocking_writer, get_fmt_layer, MultiLogFileWriter}; use tracing_appender::non_blocking::WorkerGuard; use tracing_subscriber::{reload::Handle, Layer, Registry}; @@ -43,10 +44,7 @@ pub fn init_logging_layer() -> InitLoggingLayerProduct { } fn init_logging_file_writers() -> InitLoggingFileWritersProduct { - let timestamp_log_file_name = format!( - "patch-hub_{}.log", - chrono::Local::now().format("%Y%m%d-%H%M%S") - ); + let timestamp_log_file_name = format!("patch-hub_{}.log", Local::now().format("%Y%m%d-%H%M%S")); // logging thread should be non-blocking so it does not interfere with the rest of the application let (timestamp_log_writer, timestamp_log_writer_guard) = diff --git a/src/infrastructure/monitoring/logging/multi_log_file_writer.rs b/src/infrastructure/monitoring/logging/multi_log_file_writer.rs index 87159d5..44caba6 100644 --- a/src/infrastructure/monitoring/logging/multi_log_file_writer.rs +++ b/src/infrastructure/monitoring/logging/multi_log_file_writer.rs @@ -1,9 +1,11 @@ use std::{ collections::HashMap, - fs::{File, OpenOptions}, - io::Write, + fs::{self, File, OpenOptions}, + io::{self, Write}, path::Path, sync::{Arc, Mutex}, + thread, + time::Duration, }; use tracing::{event, Level}; @@ -68,7 +70,7 @@ impl MultiLogFileWriter { if let Some(current_guard) = current_guards_by_file_name.remove(file_name) { drop(current_guard); // making sure we'll flush everything by the time we copy old file contents - std::thread::sleep(std::time::Duration::from_millis(50)); + thread::sleep(Duration::from_millis(50)); } let (file_writer, file_writer_guard) = @@ -104,7 +106,7 @@ impl MultiLogFileWriter { }; if let Some(parent_dir) = Path::new(&new_log_file_path).parent() { // Create new log dir if it doesn't exist - std::fs::create_dir_all(parent_dir).expect("to create dir"); + fs::create_dir_all(parent_dir).expect("to create dir"); } let Ok(mut new_log_file_content) = OpenOptions::new() .create(true) @@ -119,7 +121,7 @@ impl MultiLogFileWriter { return; }; - let copy_result = std::io::copy(&mut old_log_file_content, &mut new_log_file_content); + let copy_result = io::copy(&mut old_log_file_content, &mut new_log_file_content); if let Err(err) = copy_result { event!(Level::ERROR, "Could not copy old file logs: {}", err); }; @@ -127,7 +129,7 @@ impl MultiLogFileWriter { } impl Write for MultiLogFileWriter { - fn write(&mut self, buf: &[u8]) -> std::io::Result { + fn write(&mut self, buf: &[u8]) -> io::Result { for writer in self.writer_by_file_name.values() { writer .lock() @@ -138,7 +140,7 @@ impl Write for MultiLogFileWriter { Ok(buf.len()) } - fn flush(&mut self) -> std::io::Result<()> { + fn flush(&mut self) -> io::Result<()> { for writer in self.writer_by_file_name.values() { writer .lock() diff --git a/src/infrastructure/terminal.rs b/src/infrastructure/terminal.rs index ec42680..7c78a0b 100644 --- a/src/infrastructure/terminal.rs +++ b/src/infrastructure/terminal.rs @@ -15,6 +15,7 @@ use ratatui::{ Terminal, }; +use color_eyre::Result; use std::io::{self, stdout, Stdout}; /// A type alias for the terminal type used in this application @@ -34,7 +35,7 @@ pub fn restore() -> io::Result<()> { Ok(()) } -pub(crate) fn setup_user_io(terminal: &mut Terminal) -> color_eyre::Result<()> { +pub(crate) fn setup_user_io(terminal: &mut Terminal) -> Result<()> { terminal.clear()?; terminal.set_cursor_position(Position::new(0, 0))?; terminal.show_cursor()?; @@ -42,7 +43,7 @@ pub(crate) fn setup_user_io(terminal: &mut Terminal) -> color_eyr Ok(()) } -pub(crate) fn teardown_user_io(terminal: &mut Terminal) -> color_eyre::Result<()> { +pub(crate) fn teardown_user_io(terminal: &mut Terminal) -> Result<()> { enable_raw_mode()?; terminal.clear()?; Ok(()) diff --git a/src/input/actor.rs b/src/input/actor.rs index 999a8a4..71d6182 100644 --- a/src/input/actor.rs +++ b/src/input/actor.rs @@ -12,7 +12,7 @@ //! change key bindings without restarting the pump. use std::time::Duration; -use tokio::sync::mpsc; +use tokio::{select, spawn, sync::mpsc}; use crate::{ input::{ @@ -43,7 +43,7 @@ impl InputActor { channel_size = DEFAULT_INPUT_CHANNEL_SIZE, "spawning input actor" ); - tokio::spawn( + spawn( Self { rx, terminal_handle, @@ -66,7 +66,7 @@ impl InputActor { // the TerminalActor already consumed it from the OS queue. let (event_tx, mut event_rx) = mpsc::channel::(8); let terminal_handle = self.terminal_handle.clone(); - tokio::spawn(async move { + spawn(async move { loop { match terminal_handle.poll_event(EVENT_POLL_TIMEOUT).await { Ok(Some(event)) => { @@ -86,7 +86,7 @@ impl InputActor { }); loop { - tokio::select! { + select! { msg = self.rx.recv() => match msg { Some(InputMessage::SubscribeApp { tx }) => { tracing::debug!("app subscribed to input events"); diff --git a/src/lore/application/actor.rs b/src/lore/application/actor.rs index 3f41f5c..c110171 100644 --- a/src/lore/application/actor.rs +++ b/src/lore/application/actor.rs @@ -8,6 +8,7 @@ use std::ops::ControlFlow; use tokio::{ + spawn, sync::{mpsc, oneshot}, task, }; @@ -37,7 +38,7 @@ impl LoreApiActor { channel_size = DEFAULT_LORE_API_CHANNEL_SIZE, "spawning lore api actor" ); - tokio::spawn(Self::new(core, rx).run()); + spawn(Self::new(core, rx).run()); LoreApiHandle::new(tx) } diff --git a/src/lore/domain/mailing_list.rs b/src/lore/domain/mailing_list.rs index 40aead8..571b9bc 100644 --- a/src/lore/domain/mailing_list.rs +++ b/src/lore/domain/mailing_list.rs @@ -1,3 +1,5 @@ +use std::cmp::Ordering; + use derive_getters::Getters; use serde::{Deserialize, Serialize}; @@ -17,13 +19,13 @@ impl MailingList { } impl Ord for MailingList { - fn cmp(&self, other: &Self) -> std::cmp::Ordering { + fn cmp(&self, other: &Self) -> Ordering { self.name.cmp(&other.name) } } impl PartialOrd for MailingList { - fn partial_cmp(&self, other: &Self) -> Option { + fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) } } diff --git a/src/lore/domain/patch.rs b/src/lore/domain/patch.rs index add0579..996d749 100644 --- a/src/lore/domain/patch.rs +++ b/src/lore/domain/patch.rs @@ -4,7 +4,7 @@ use derive_getters::Getters; use regex::Regex; use serde::{Deserialize, Serialize}; -use std::fmt::Display; +use std::fmt::{self, Display, Formatter}; #[derive(Getters, Serialize, Deserialize, Debug, Clone)] pub struct PatchFeed { @@ -39,7 +39,7 @@ pub struct Author { } impl Display for Author { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { write!(f, "{} <{}>", self.name, self.email)?; Ok(()) } diff --git a/src/lore/infrastructure/persistence.rs b/src/lore/infrastructure/persistence.rs index 67a94a7..b59f2c0 100644 --- a/src/lore/infrastructure/persistence.rs +++ b/src/lore/infrastructure/persistence.rs @@ -1,4 +1,6 @@ use mockall::automock; +use serde::{de::DeserializeOwned, Serialize}; +use serde_json::{from_reader, to_writer}; use std::{ collections::{HashMap, HashSet}, @@ -58,7 +60,7 @@ impl FileLorePersistence { } } - fn atomic_write_json( + fn atomic_write_json( &self, value: &T, path: &str, @@ -70,15 +72,15 @@ impl FileLorePersistence { let tmp_path = format!("{path}.tmp"); { let writer = self.fs.create_writer(Path::new(&tmp_path))?; - serde_json::to_writer(writer, value).map_err(io::Error::from)?; + to_writer(writer, value).map_err(io::Error::from)?; } self.fs.rename(Path::new(&tmp_path), Path::new(path))?; Ok(()) } - fn read_json(&self, path: &str) -> Result { + fn read_json(&self, path: &str) -> Result { let reader = self.fs.open_bufreader(Path::new(path))?; - serde_json::from_reader(reader) + from_reader(reader) .map_err(io::Error::from) .map_err(FileSystemError::from) } diff --git a/src/main.rs b/src/main.rs index c6231ba..211efb6 100644 --- a/src/main.rs +++ b/src/main.rs @@ -13,14 +13,14 @@ mod ui; use app::{actor::AppActor, dependencies::check_external_deps, App}; use clap::Parser; use cli::Cli; -use color_eyre::eyre::eyre; +use color_eyre::{eyre::eyre, Result}; use config::{bootstrap_parts, ConfigActor}; use infrastructure::{ env::OsEnv, - file_system::OsFileSystem, + file_system::{FileSystemTrait, OsFileSystem}, monitoring::{init_monitoring, InitMonitoringProduct}, net::UreqNetClient, - shell::OsShell, + shell::{OsShell, ShellTrait}, terminal::init, }; use input::{actor::InputActor, event::InputEvent}; @@ -41,7 +41,7 @@ use tracing::{event, Level}; use ui::actor::UiActor; #[tokio::main] -async fn main() -> color_eyre::Result<()> { +async fn main() -> Result<()> { // file writer guards should be propagated to main() so the logging thread lives enough let InitMonitoringProduct { logging_guards_by_file_name, @@ -78,8 +78,8 @@ async fn main() -> color_eyre::Result<()> { // Build shared infrastructure dependencies for LoreService let net = Arc::new(UreqNetClient::new()); - let fs_arc: Arc = Arc::new(OsFileSystem); - let shell_arc: Arc = Arc::new(OsShell); + let fs_arc: Arc = Arc::new(OsFileSystem); + let shell_arc: Arc = Arc::new(OsShell); let gateway = Arc::new(HttpLoreGateway::new(net)); let persistence = Arc::new(FileLorePersistence::new( diff --git a/src/render/actor.rs b/src/render/actor.rs index 65dc87e..fa1b13b 100644 --- a/src/render/actor.rs +++ b/src/render/actor.rs @@ -9,6 +9,7 @@ use std::ops::ControlFlow; use tokio::{ + spawn, sync::{mpsc, oneshot}, task, }; @@ -40,7 +41,7 @@ impl RenderActor { channel_size = DEFAULT_RENDER_CHANNEL_SIZE, "spawning render actor" ); - tokio::spawn(Self::new(core, rx).run()); + spawn(Self::new(core, rx).run()); RenderHandle::new(tx) } diff --git a/src/render/cover_renderer.rs b/src/render/cover_renderer.rs index e136c70..fd36de3 100644 --- a/src/render/cover_renderer.rs +++ b/src/render/cover_renderer.rs @@ -1,17 +1,13 @@ use tracing::{event, Level}; -use color_eyre::eyre::eyre; +use color_eyre::{eyre::eyre, Result}; use crate::{ infrastructure::shell::{ShellCommand, ShellTrait}, render_prefs::CoverRenderer, }; -pub fn render_cover( - shell: &dyn ShellTrait, - raw: &str, - renderer: &CoverRenderer, -) -> color_eyre::Result { +pub fn render_cover(shell: &dyn ShellTrait, raw: &str, renderer: &CoverRenderer) -> Result { let text = match renderer { CoverRenderer::Default => Ok(raw.to_string()), CoverRenderer::Bat => bat_cover_renderer(shell, raw), @@ -25,7 +21,7 @@ pub fn render_cover( /// # Errors /// /// If bat isn't installed or if the command fails, an error will be returned. -fn bat_cover_renderer(shell: &dyn ShellTrait, patch: &str) -> color_eyre::Result { +fn bat_cover_renderer(shell: &dyn ShellTrait, patch: &str) -> Result { let cmd = ShellCommand::new("bat").args(["-pp", "-f", "-l", "mbx"]); let out = shell diff --git a/src/render/patch_renderer.rs b/src/render/patch_renderer.rs index a43ccee..62e50d1 100644 --- a/src/render/patch_renderer.rs +++ b/src/render/patch_renderer.rs @@ -1,4 +1,4 @@ -use color_eyre::eyre::eyre; +use color_eyre::{eyre::eyre, Result}; use tracing::{event, Level}; use crate::{ @@ -23,7 +23,7 @@ pub fn render_patch_preview( shell: &dyn ShellTrait, raw: &str, renderer: &PatchRenderer, -) -> color_eyre::Result { +) -> Result { let text = match renderer { PatchRenderer::Default => Ok(raw.to_string()), PatchRenderer::Bat => bat_patch_renderer(shell, raw), @@ -43,7 +43,7 @@ pub fn render_patch_preview( /// # Tests /// /// [tests::test_bat_patch_renderer] -fn bat_patch_renderer(shell: &dyn ShellTrait, patch: &str) -> color_eyre::Result { +fn bat_patch_renderer(shell: &dyn ShellTrait, patch: &str) -> Result { let cleaned_patch = clean_patch_for_preview(patch); let cmd = ShellCommand::new("bat").args(["-pp", "-f", "-l", "patch"]); @@ -67,7 +67,7 @@ fn bat_patch_renderer(shell: &dyn ShellTrait, patch: &str) -> color_eyre::Result /// # Tests /// /// [tests::test_delta_patch_renderer] -fn delta_patch_renderer(shell: &dyn ShellTrait, patch: &str) -> color_eyre::Result { +fn delta_patch_renderer(shell: &dyn ShellTrait, patch: &str) -> Result { let cleaned_patch = clean_patch_for_preview(patch); let cmd = ShellCommand::new("delta").args([ @@ -103,7 +103,7 @@ fn delta_patch_renderer(shell: &dyn ShellTrait, patch: &str) -> color_eyre::Resu /// # Tests /// /// [tests::test_diff_so_fancy_renderer] -fn diff_so_fancy_renderer(shell: &dyn ShellTrait, patch: &str) -> color_eyre::Result { +fn diff_so_fancy_renderer(shell: &dyn ShellTrait, patch: &str) -> Result { let cleaned_patch = clean_patch_for_preview(patch); let cmd = ShellCommand::new("diff-so-fancy"); diff --git a/src/render_prefs.rs b/src/render_prefs.rs index 1e77e4d..0216b2e 100644 --- a/src/render_prefs.rs +++ b/src/render_prefs.rs @@ -1,7 +1,8 @@ //! Patch and cover renderer preferences (serde + display), shared by config and rendering. +use std::fmt::{self, Display, Formatter}; + use serde::{Deserialize, Serialize}; -use std::fmt::Display; #[derive(Debug, Serialize, Deserialize, Clone, Copy, Default)] pub enum PatchRenderer { @@ -39,7 +40,7 @@ impl From<&str> for PatchRenderer { } impl Display for PatchRenderer { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { match self { PatchRenderer::Default => write!(f, "default"), PatchRenderer::Bat => write!(f, "bat"), @@ -77,7 +78,7 @@ impl From<&str> for CoverRenderer { } impl Display for CoverRenderer { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { match self { CoverRenderer::Default => write!(f, "default"), CoverRenderer::Bat => write!(f, "bat"), diff --git a/src/terminal/actor.rs b/src/terminal/actor.rs index c0746af..510b6bd 100644 --- a/src/terminal/actor.rs +++ b/src/terminal/actor.rs @@ -6,6 +6,7 @@ //! e.g. crossterm) is moved into the actor at spawn time so no other component //! holds the terminal directly. use tokio::{ + spawn, sync::{mpsc, oneshot}, task, }; @@ -38,7 +39,7 @@ impl TerminalActor { channel_size = DEFAULT_TERMINAL_CHANNEL_SIZE, "spawning terminal actor" ); - tokio::spawn(Self::new(session, rx).run()); + spawn(Self::new(session, rx).run()); TerminalHandle::new(tx) } diff --git a/src/terminal/errors.rs b/src/terminal/errors.rs index 673256f..3c845a2 100644 --- a/src/terminal/errors.rs +++ b/src/terminal/errors.rs @@ -1,10 +1,12 @@ +use std::io; + use thiserror::Error; /// Failures at the terminal actor/session boundary. #[derive(Debug, Error)] pub enum TerminalError { #[error("terminal I/O error: {0}")] - Io(#[from] std::io::Error), + Io(#[from] io::Error), #[error("terminal session error: {0}")] Session(String), #[error("terminal actor unavailable: {0}")] diff --git a/src/terminal/session.rs b/src/terminal/session.rs index 72c9895..db4b4d9 100644 --- a/src/terminal/session.rs +++ b/src/terminal/session.rs @@ -1,4 +1,4 @@ -use std::time::Duration; +use std::time::{Duration, Instant}; use mockall::automock; use ratatui::crossterm::event::{self, Event, KeyCode, KeyEventKind}; @@ -116,7 +116,7 @@ fn wait_for_key_press_from_session( key: KeyCode, timeout: Duration, ) -> TerminalResult { - let started_at = std::time::Instant::now(); + let started_at = Instant::now(); while started_at.elapsed() < timeout { let elapsed = started_at.elapsed(); diff --git a/src/ui/actor.rs b/src/ui/actor.rs index 032c426..0a17014 100644 --- a/src/ui/actor.rs +++ b/src/ui/actor.rs @@ -8,9 +8,12 @@ //! 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 std::{mem, ops::ControlFlow}; -use tokio::sync::{mpsc, oneshot}; +use tokio::{ + spawn, + sync::{mpsc, oneshot}, +}; use crate::ui::{ core::UiCore, @@ -36,7 +39,7 @@ impl UiActor { pub fn spawn() -> UiHandle { let (tx, rx) = mpsc::channel(DEFAULT_UI_CHANNEL_SIZE); tracing::debug!(channel_size = DEFAULT_UI_CHANNEL_SIZE, "spawning ui actor"); - tokio::spawn(Self::new(rx).run()); + spawn(Self::new(rx).run()); UiHandle::new(tx) } @@ -57,7 +60,7 @@ impl UiActor { match message { UiMessage::BuildScene { app_view, reply_to } => { tracing::debug!( - screen = ?std::mem::discriminant(&app_view.screen), + screen = ?mem::discriminant(&app_view.screen), has_popup = app_view.popup.is_some(), "building ui scene" ); diff --git a/src/ui/screens/bookmarked.rs b/src/ui/screens/bookmarked.rs index 0113d58..357b121 100644 --- a/src/ui/screens/bookmarked.rs +++ b/src/ui/screens/bookmarked.rs @@ -2,7 +2,7 @@ use ratatui::{ layout::Rect, style::{Color, Modifier, Style}, text::{Line, Span}, - widgets::{Block, Borders, HighlightSpacing, List, ListItem, ListState}, + widgets::{Block, BorderType, Borders, HighlightSpacing, List, ListItem, ListState}, Frame, }; @@ -45,7 +45,7 @@ pub fn paint(f: &mut Frame, scene: &BookmarkedScene, chunk: Rect) { let list_block = Block::default() .borders(Borders::ALL) - .border_type(ratatui::widgets::BorderType::Double) + .border_type(BorderType::Double) .style(Style::default()); let list = List::new(list_items) diff --git a/src/ui/screens/details.rs b/src/ui/screens/details.rs index 47bb90c..824fb4d 100644 --- a/src/ui/screens/details.rs +++ b/src/ui/screens/details.rs @@ -2,7 +2,7 @@ use ratatui::{ layout::{Constraint, Direction, Layout, Rect}, style::{Color, Modifier, Style}, text::{Line, Span}, - widgets::{Block, Borders, Padding, Paragraph, Wrap}, + widgets::{Block, BorderType, Borders, Padding, Paragraph, Wrap}, Frame, }; @@ -139,7 +139,7 @@ fn paint_details_and_actions( .block( Block::default() .borders(Borders::ALL) - .border_type(ratatui::widgets::BorderType::Double) + .border_type(BorderType::Double) .title(Line::styled(" Details ", Style::default().fg(Color::Green)).left_aligned()) .padding(Padding::vertical(1)), ) @@ -200,7 +200,7 @@ fn paint_details_and_actions( .block( Block::default() .borders(Borders::ALL) - .border_type(ratatui::widgets::BorderType::Double) + .border_type(BorderType::Double) .title(Line::styled(" Actions ", Style::default().fg(Color::Green)).left_aligned()) .padding(Padding::vertical(1)), ) @@ -216,7 +216,7 @@ fn paint_preview(f: &mut Frame, scene: &PatchsetDetailsScene, chunk: Rect) { .block( Block::default() .borders(Borders::ALL) - .border_type(ratatui::widgets::BorderType::Double) + .border_type(BorderType::Double) .title( Line::styled( scene.preview_title.clone(), diff --git a/src/ui/screens/latest.rs b/src/ui/screens/latest.rs index ddbb0fa..5429bfb 100644 --- a/src/ui/screens/latest.rs +++ b/src/ui/screens/latest.rs @@ -2,7 +2,7 @@ use ratatui::{ layout::Rect, style::{Color, Modifier, Style}, text::{Line, Span}, - widgets::{Block, Borders, HighlightSpacing, List, ListItem, ListState}, + widgets::{Block, BorderType, Borders, HighlightSpacing, List, ListItem, ListState}, Frame, }; @@ -45,7 +45,7 @@ pub fn paint(f: &mut Frame, scene: &LatestScene, chunk: Rect) { let list_block = Block::default() .borders(Borders::ALL) - .border_type(ratatui::widgets::BorderType::Double) + .border_type(BorderType::Double) .style(Style::default()); let list = List::new(list_items) diff --git a/src/ui/screens/mailing_list.rs b/src/ui/screens/mailing_list.rs index c18c685..ef0c0dc 100644 --- a/src/ui/screens/mailing_list.rs +++ b/src/ui/screens/mailing_list.rs @@ -2,7 +2,7 @@ use ratatui::{ layout::Rect, style::{Color, Modifier, Style}, text::{Line, Span}, - widgets::{Block, Borders, HighlightSpacing, List, ListItem, ListState}, + widgets::{Block, BorderType, Borders, HighlightSpacing, List, ListItem, ListState}, Frame, }; @@ -44,7 +44,7 @@ pub fn paint(f: &mut Frame, scene: &MailingListScene, chunk: Rect) { let list_block = Block::default() .borders(Borders::ALL) - .border_type(ratatui::widgets::BorderType::Double) + .border_type(BorderType::Double) .style(Style::default()); let list = List::new(list_items) diff --git a/src/ui/screens/popup.rs b/src/ui/screens/popup.rs index 23e7be8..b9259a7 100644 --- a/src/ui/screens/popup.rs +++ b/src/ui/screens/popup.rs @@ -1,5 +1,5 @@ use ratatui::{ - layout::Alignment, + layout::{Alignment, Rect}, style::{Color, Modifier, Style, Stylize}, text::Line, widgets::{Block, BorderType, Borders, Clear, Paragraph, Wrap}, @@ -48,7 +48,7 @@ pub fn build_scene(vm: &PopupViewModel) -> PopupScene { // Painter // --------------------------------------------------------------------------- -pub fn paint(f: &mut Frame, scene: &PopupScene, chunk: ratatui::layout::Rect) { +pub fn paint(f: &mut Frame, scene: &PopupScene, chunk: Rect) { match &scene.body { PopupBody::Text(body) => paint_info(f, &scene.title, body, scene.scroll_offset, chunk), PopupBody::Keybinds { @@ -77,13 +77,7 @@ pub fn paint(f: &mut Frame, scene: &PopupScene, chunk: ratatui::layout::Rect) { } } -fn paint_info( - f: &mut Frame, - title: &str, - body: &str, - scroll: (u16, u16), - chunk: ratatui::layout::Rect, -) { +fn paint_info(f: &mut Frame, title: &str, body: &str, scroll: (u16, u16), chunk: Rect) { let bold_blue = Style::default() .add_modifier(Modifier::BOLD) .fg(Color::Blue); @@ -112,7 +106,7 @@ fn paint_help( description: Option<&str>, formatted_keybinds: &str, scroll: (u16, u16), - chunk: ratatui::layout::Rect, + chunk: Rect, ) { let block = Block::default() .title(title.to_string()) @@ -148,7 +142,7 @@ fn paint_review_trailers( tested_by: &str, acked_by: &str, scroll: (u16, u16), - chunk: ratatui::layout::Rect, + chunk: Rect, ) { let header_style = Style::default() .fg(Color::Cyan) From 17156cb2d6840d9f842d2dfc911b13e6cc3fe4ae Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 9 Jul 2026 15:21:56 -0300 Subject: [PATCH 07/18] style: remove decorative section divider comments This commit removes artificial section divider comments from app and UI modules. Normal blank-line spacing between types and functions is preserved without changing code behavior. This commit is part of the architecture's refactoring phase 15. Signed-off-by: lorenzoberts --- src/app/popup.rs | 18 ----------------- src/app/view_model.rs | 29 --------------------------- src/ui/scene.rs | 36 ---------------------------------- src/ui/screens/bookmarked.rs | 12 ------------ src/ui/screens/details.rs | 13 ------------ src/ui/screens/edit_config.rs | 12 ------------ src/ui/screens/latest.rs | 12 ------------ src/ui/screens/mailing_list.rs | 13 ------------ src/ui/screens/popup.rs | 9 --------- 9 files changed, 154 deletions(-) diff --git a/src/app/popup.rs b/src/app/popup.rs index c066a7a..75157e4 100644 --- a/src/app/popup.rs +++ b/src/app/popup.rs @@ -11,10 +11,6 @@ use crate::{ lore::domain::patch::Author, }; -// --------------------------------------------------------------------------- -// Main enum -// --------------------------------------------------------------------------- - /// Concrete, cloneable popup state stored in `AppState`. #[derive(Clone, Debug)] pub enum AppPopup { @@ -44,10 +40,6 @@ pub enum AppPopup { } impl AppPopup { - // ----------------------------------------------------------------------- - // Factories - // ----------------------------------------------------------------------- - /// Create an informational text popup. pub fn info(title: impl Into, body: impl Into) -> Self { let title = title.into(); @@ -111,11 +103,6 @@ impl AppPopup { dimensions: (50, 40), } } - - // ----------------------------------------------------------------------- - // Input handling - // ----------------------------------------------------------------------- - /// Advance scroll position in response to a navigation input. /// /// All popup variants share identical two-axis scroll semantics. @@ -155,11 +142,6 @@ impl AppPopup { } } } - -// --------------------------------------------------------------------------- -// Help builder -// --------------------------------------------------------------------------- - /// Fluent builder for `AppPopup::Help`. /// /// Mirrors the API of the old `HelpPopUpBuilder` so handler call sites change diff --git a/src/app/view_model.rs b/src/app/view_model.rs index c130f0a..1a5ecfc 100644 --- a/src/app/view_model.rs +++ b/src/app/view_model.rs @@ -16,10 +16,6 @@ use super::{ state::AppState, }; -// --------------------------------------------------------------------------- -// Shared row / helper types -// --------------------------------------------------------------------------- - /// One mailing list entry shown in the selection list. #[derive(Clone, Debug)] pub struct MailingListEntry { @@ -71,10 +67,6 @@ pub struct ConfigEntryRow { pub edit_cursor_value: String, } -// --------------------------------------------------------------------------- -// Per-screen view models -// --------------------------------------------------------------------------- - #[derive(Clone, Debug)] pub struct MailingListSelectionViewModel { pub entries: Vec, @@ -129,10 +121,6 @@ pub struct EditConfigViewModel { pub is_editing_mode: bool, } -// --------------------------------------------------------------------------- -// Popup view model -// --------------------------------------------------------------------------- - #[derive(Clone, Debug)] pub enum PopupViewBody { Text(String), @@ -156,10 +144,6 @@ pub struct PopupViewModel { pub dimensions: (u16, u16), } -// --------------------------------------------------------------------------- -// Discriminated screen enum -// --------------------------------------------------------------------------- - #[derive(Clone, Debug)] pub enum ScreenViewModel { MailingListSelection(MailingListSelectionViewModel), @@ -169,10 +153,6 @@ pub enum ScreenViewModel { EditConfig(EditConfigViewModel), } -// --------------------------------------------------------------------------- -// Top-level view model -// --------------------------------------------------------------------------- - /// Owned, typed projection of [`AppState`] for one TUI frame. /// /// Built by [`project_state`]; consumed by `UiCore::build_scene`. @@ -182,10 +162,6 @@ pub struct AppViewModel { pub popup: Option, } -// --------------------------------------------------------------------------- -// Projection – public entry point -// --------------------------------------------------------------------------- - /// Projects `state` into an owned [`AppViewModel`]. /// /// Called via [`super::App::present`]. @@ -194,11 +170,6 @@ pub fn project_state(state: &AppState) -> AppViewModel { let popup = state.popup.as_ref().map(project_popup); AppViewModel { screen, popup } } - -// --------------------------------------------------------------------------- -// Private per-screen projectors -// --------------------------------------------------------------------------- - fn project_screen(state: &AppState) -> ScreenViewModel { match state.navigation.current_screen { CurrentScreen::MailingListSelection => { diff --git a/src/ui/scene.rs b/src/ui/scene.rs index 65483a5..b6c6998 100644 --- a/src/ui/scene.rs +++ b/src/ui/scene.rs @@ -13,10 +13,6 @@ pub use crate::app::view_model::{ ConfigEntryRow, MailingListEntry, PatchSummaryRow, TagTrailerCounts, }; -// --------------------------------------------------------------------------- -// Mailing-list selection -// --------------------------------------------------------------------------- - /// Scene for the mailing-list selection screen. #[derive(Clone, Debug)] pub struct MailingListScene { @@ -24,10 +20,6 @@ pub struct MailingListScene { pub highlighted_index: usize, } -// --------------------------------------------------------------------------- -// Bookmarked patchsets -// --------------------------------------------------------------------------- - /// Scene for the bookmarked-patchsets screen. #[derive(Clone, Debug)] pub struct BookmarkedScene { @@ -35,10 +27,6 @@ pub struct BookmarkedScene { pub selected_index: usize, } -// --------------------------------------------------------------------------- -// Latest patchsets -// --------------------------------------------------------------------------- - /// Scene for the latest-patchsets screen. #[derive(Clone, Debug)] pub struct LatestScene { @@ -46,10 +34,6 @@ pub struct LatestScene { pub selected_index: usize, } -// --------------------------------------------------------------------------- -// Patchset details -// --------------------------------------------------------------------------- - /// Scene for the patchset-details-and-actions screen. #[derive(Clone, Debug)] pub struct PatchsetDetailsScene { @@ -78,20 +62,12 @@ pub struct PatchsetDetailsScene { pub is_current_patch_reply_staged: bool, } -// --------------------------------------------------------------------------- -// Edit config -// --------------------------------------------------------------------------- - /// Scene for the edit-configuration screen. #[derive(Clone, Debug)] pub struct EditConfigScene { pub entries: Vec, } -// --------------------------------------------------------------------------- -// Discriminated body -// --------------------------------------------------------------------------- - /// Which screen's scene the body carries. #[derive(Clone, Debug)] pub enum UiBody { @@ -102,10 +78,6 @@ pub enum UiBody { EditConfig(EditConfigScene), } -// --------------------------------------------------------------------------- -// Navigation bar -// --------------------------------------------------------------------------- - /// Pre-computed navigation-bar content. /// /// `mode_spans` is a list of styled text spans that together form the left @@ -117,10 +89,6 @@ pub struct NavigationBarScene { pub keys_hint: Span<'static>, } -// --------------------------------------------------------------------------- -// Popup -// --------------------------------------------------------------------------- - /// Structured body for each popup variant. #[derive(Clone, Debug)] pub enum PopupBody { @@ -149,10 +117,6 @@ pub struct PopupScene { pub dimensions: (u16, u16), } -// --------------------------------------------------------------------------- -// Top-level scene -// --------------------------------------------------------------------------- - /// The complete, paint-ready visual snapshot for one TUI frame. #[derive(Clone, Debug)] pub struct UiScene { diff --git a/src/ui/screens/bookmarked.rs b/src/ui/screens/bookmarked.rs index 357b121..467002c 100644 --- a/src/ui/screens/bookmarked.rs +++ b/src/ui/screens/bookmarked.rs @@ -8,10 +8,6 @@ use ratatui::{ use crate::{app::view_model::BookmarkedViewModel, ui::scene::BookmarkedScene}; -// --------------------------------------------------------------------------- -// Builder -// --------------------------------------------------------------------------- - pub fn build_scene(vm: &BookmarkedViewModel) -> BookmarkedScene { BookmarkedScene { rows: vm.rows.clone(), @@ -19,10 +15,6 @@ pub fn build_scene(vm: &BookmarkedViewModel) -> BookmarkedScene { } } -// --------------------------------------------------------------------------- -// Painter -// --------------------------------------------------------------------------- - pub fn paint(f: &mut Frame, scene: &BookmarkedScene, chunk: Rect) { let mut list_items = Vec::::new(); @@ -65,10 +57,6 @@ pub fn paint(f: &mut Frame, scene: &BookmarkedScene, chunk: Rect) { f.render_stateful_widget(list, chunk, &mut list_state); } -// --------------------------------------------------------------------------- -// Navigation-bar helpers -// --------------------------------------------------------------------------- - pub fn mode_spans() -> Vec> { vec![Span::styled( "Bookmarked Patchsets", diff --git a/src/ui/screens/details.rs b/src/ui/screens/details.rs index 824fb4d..cf1dc23 100644 --- a/src/ui/screens/details.rs +++ b/src/ui/screens/details.rs @@ -10,11 +10,6 @@ use crate::{ app::view_model::PatchsetDetailsViewModel, ui::scene::{PatchsetDetailsScene, TagTrailerCounts}, }; - -// --------------------------------------------------------------------------- -// Builder -// --------------------------------------------------------------------------- - pub fn build_scene(vm: &PatchsetDetailsViewModel) -> PatchsetDetailsScene { PatchsetDetailsScene { patch_title: vm.patch_title.clone(), @@ -36,10 +31,6 @@ pub fn build_scene(vm: &PatchsetDetailsViewModel) -> PatchsetDetailsScene { } } -// --------------------------------------------------------------------------- -// Painter -// --------------------------------------------------------------------------- - pub fn paint(f: &mut Frame, scene: &PatchsetDetailsScene, chunk: Rect) { if scene.preview_fullscreen { paint_preview(f, scene, chunk); @@ -232,10 +223,6 @@ fn paint_preview(f: &mut Frame, scene: &PatchsetDetailsScene, chunk: Rect) { f.render_widget(patch_preview, chunk); } -// --------------------------------------------------------------------------- -// Navigation-bar helpers -// --------------------------------------------------------------------------- - pub fn mode_spans() -> Vec> { vec![Span::styled( "Patchset Details and Actions", diff --git a/src/ui/screens/edit_config.rs b/src/ui/screens/edit_config.rs index 4d0c4d5..fff8895 100644 --- a/src/ui/screens/edit_config.rs +++ b/src/ui/screens/edit_config.rs @@ -8,20 +8,12 @@ use ratatui::{ use crate::{app::view_model::EditConfigViewModel, ui::scene::EditConfigScene}; -// --------------------------------------------------------------------------- -// Builder -// --------------------------------------------------------------------------- - pub fn build_scene(vm: &EditConfigViewModel) -> EditConfigScene { EditConfigScene { entries: vm.entries.clone(), } } -// --------------------------------------------------------------------------- -// Painter -// --------------------------------------------------------------------------- - pub fn paint(f: &mut Frame, scene: &EditConfigScene, chunk: Rect) { let mut constraints = Vec::new(); @@ -71,10 +63,6 @@ pub fn paint(f: &mut Frame, scene: &EditConfigScene, chunk: Rect) { } } -// --------------------------------------------------------------------------- -// Navigation-bar helpers -// --------------------------------------------------------------------------- - pub fn mode_spans(vm: &EditConfigViewModel) -> Vec> { vec![if vm.is_editing_mode { Span::styled("Editing...", Style::default().fg(Color::LightYellow)) diff --git a/src/ui/screens/latest.rs b/src/ui/screens/latest.rs index 5429bfb..0b1dc46 100644 --- a/src/ui/screens/latest.rs +++ b/src/ui/screens/latest.rs @@ -8,10 +8,6 @@ use ratatui::{ use crate::{app::view_model::LatestPatchsetsViewModel, ui::scene::LatestScene}; -// --------------------------------------------------------------------------- -// Builder -// --------------------------------------------------------------------------- - pub fn build_scene(vm: &LatestPatchsetsViewModel) -> LatestScene { LatestScene { rows: vm.rows.clone(), @@ -19,10 +15,6 @@ pub fn build_scene(vm: &LatestPatchsetsViewModel) -> LatestScene { } } -// --------------------------------------------------------------------------- -// Painter -// --------------------------------------------------------------------------- - pub fn paint(f: &mut Frame, scene: &LatestScene, chunk: Rect) { let mut list_items = Vec::::new(); @@ -65,10 +57,6 @@ pub fn paint(f: &mut Frame, scene: &LatestScene, chunk: Rect) { f.render_stateful_widget(list, chunk, &mut list_state); } -// --------------------------------------------------------------------------- -// Navigation-bar helpers -// --------------------------------------------------------------------------- - pub fn mode_spans(vm: &LatestPatchsetsViewModel) -> Vec> { vec![Span::styled( format!( diff --git a/src/ui/screens/mailing_list.rs b/src/ui/screens/mailing_list.rs index ef0c0dc..868cf44 100644 --- a/src/ui/screens/mailing_list.rs +++ b/src/ui/screens/mailing_list.rs @@ -10,11 +10,6 @@ use crate::{ app::view_model::{MailingListSelectionViewModel, TargetListStatus}, ui::scene::MailingListScene, }; - -// --------------------------------------------------------------------------- -// Builder -// --------------------------------------------------------------------------- - pub fn build_scene(vm: &MailingListSelectionViewModel) -> MailingListScene { MailingListScene { entries: vm.entries.clone(), @@ -22,10 +17,6 @@ pub fn build_scene(vm: &MailingListSelectionViewModel) -> MailingListScene { } } -// --------------------------------------------------------------------------- -// Painter -// --------------------------------------------------------------------------- - pub fn paint(f: &mut Frame, scene: &MailingListScene, chunk: Rect) { let mut list_items = Vec::::new(); @@ -64,10 +55,6 @@ pub fn paint(f: &mut Frame, scene: &MailingListScene, chunk: Rect) { f.render_stateful_widget(list, chunk, &mut list_state); } -// --------------------------------------------------------------------------- -// Navigation-bar helpers -// --------------------------------------------------------------------------- - pub fn mode_spans(vm: &MailingListSelectionViewModel) -> Vec> { let text_area = match vm.target_list_status { TargetListStatus::Empty => { diff --git a/src/ui/screens/popup.rs b/src/ui/screens/popup.rs index b9259a7..41a0e01 100644 --- a/src/ui/screens/popup.rs +++ b/src/ui/screens/popup.rs @@ -10,11 +10,6 @@ use crate::{ app::view_model::{PopupViewBody, PopupViewModel}, ui::scene::{PopupBody, PopupScene}, }; - -// --------------------------------------------------------------------------- -// Builder -// --------------------------------------------------------------------------- - pub fn build_scene(vm: &PopupViewModel) -> PopupScene { let body = match &vm.body { PopupViewBody::Text(text) => PopupBody::Text(text.clone()), @@ -44,10 +39,6 @@ pub fn build_scene(vm: &PopupViewModel) -> PopupScene { } } -// --------------------------------------------------------------------------- -// Painter -// --------------------------------------------------------------------------- - pub fn paint(f: &mut Frame, scene: &PopupScene, chunk: Rect) { match &scene.body { PopupBody::Text(body) => paint_info(f, &scene.title, body, scene.scroll_offset, chunk), From 4a7fcd648d4dc2e03ba9b2bac3ceddea73f627d8 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 9 Jul 2026 15:41:05 -0300 Subject: [PATCH 08/18] refactor(app): extract patch apply action service This commit moves patch-apply side effects out of PatchsetDetailsState into a focused application action service. App still builds the request and sets the resulting popup, but git command execution and failure handling live behind a named action boundary. This commit is part of the architecture's refactoring phase 15. Signed-off-by: lorenzoberts --- src/app/actions/apply.rs | 468 +++++++++++++++++++++++++++++ src/app/actions/mod.rs | 1 + src/app/mod.rs | 21 +- src/app/screens/details_actions.rs | 260 ---------------- 4 files changed, 483 insertions(+), 267 deletions(-) create mode 100644 src/app/actions/apply.rs create mode 100644 src/app/actions/mod.rs diff --git a/src/app/actions/apply.rs b/src/app/actions/apply.rs new file mode 100644 index 0000000..6db047b --- /dev/null +++ b/src/app/actions/apply.rs @@ -0,0 +1,468 @@ +use std::path::Path; + +use chrono::Utc; + +use crate::{ + config::{ConfigSnapshot, KernelTree}, + infrastructure::{ + file_system::FileSystemTrait, + shell::{ShellCommand, ShellTrait}, + }, +}; + +pub(crate) struct ApplyPatchsetRequest { + pub patch_title: String, + pub patchset_path: String, +} + +pub(crate) fn apply_patchset( + request: &ApplyPatchsetRequest, + fs: &dyn FileSystemTrait, + shell: &dyn ShellTrait, + config: &ConfigSnapshot, +) -> Result { + let kernel_tree = validate_kernel_tree(fs, config)?; + check_git_state(fs, shell, kernel_tree)?; + + let original_branch = get_current_branch(shell, kernel_tree)?; + let target_branch = create_target_branch(shell, kernel_tree, config)?; + + let git_am_result = run_git_am(request, shell, kernel_tree, config); + switch_to_branch(shell, kernel_tree, &original_branch)?; + + match git_am_result { + Ok(_) => Ok(format!( + " Patchset '{}' applied successfully!\n\n - Kernel Tree: '{}'\n\n - Base Branch: '{}'\n\n - Applied branch: '{}'", + request.patch_title, + kernel_tree.path(), + kernel_tree.branch(), + &target_branch + )), + Err(e) => Err(format!(" `git am` failed\n{}{}", &original_branch, e)), + } +} + +fn validate_kernel_tree<'a>( + fs: &dyn FileSystemTrait, + config: &'a ConfigSnapshot, +) -> Result<&'a KernelTree, String> { + let kernel_tree_id = if let Some(target) = config.target_kernel_tree() { + target + } else { + return Err("target kernel tree unset".to_string()); + }; + + let kernel_tree = if let Some(tree) = config.get_kernel_tree(kernel_tree_id) { + tree + } else { + return Err(format!("invalid target kernel tree '{kernel_tree_id}'")); + }; + + let kernel_tree_path = Path::new(kernel_tree.path()); + if !fs.is_dir(kernel_tree_path) { + return Err(format!("{} isn't a directory", kernel_tree.path())); + } else if !fs.is_dir(&kernel_tree_path.join(".git")) { + return Err(format!("{} isn't a git repository", kernel_tree.path())); + } + + Ok(kernel_tree) +} + +fn check_git_state( + fs: &dyn FileSystemTrait, + shell: &dyn ShellTrait, + kernel_tree: &KernelTree, +) -> Result<(), String> { + let kernel_tree_path = Path::new(kernel_tree.path()); + + if fs.is_dir(&kernel_tree_path.join(".git/rebase-merge")) { + return Err("rebase in progress. \nrun `git rebase --abort` before continuing".to_string()); + } else if fs.is_file(&kernel_tree_path.join(".git/MERGE_HEAD")) { + return Err("merge in progress. \nrun `git merge --abort` before continuing".to_string()); + } else if fs.is_file(&kernel_tree_path.join(".git/BISECT_LOG")) { + return Err("bisect in progress. \nrun `git bisect reset` before continuing".to_string()); + } else if fs.is_dir(&kernel_tree_path.join(".git/rebase-apply")) { + return Err( + "`git am` already in progress. \nrun `git am --abort` before continuing".to_string(), + ); + } + + let status_out = shell + .execute( + &ShellCommand::new("git") + .arg("-C") + .arg(kernel_tree.path()) + .args(["status", "--porcelain"]), + ) + .map_err(|e| format!("failed to check git status {e}"))?; + + let status_output = String::from_utf8_lossy(&status_out.stdout); + if !status_output.is_empty() { + return Err(format!( + "there are staged and/or unstaged changes\n{status_output}" + )); + } + + let show_ref_out = shell + .execute( + &ShellCommand::new("git") + .arg("-C") + .arg(kernel_tree.path()) + .args(["show-ref", "--verify", "--quiet"]) + .arg(format!("refs/heads/{}", kernel_tree.branch())), + ) + .map_err(|e| format!("failed to verify branch: {e}"))?; + + if !show_ref_out.success { + return Err(format!( + "invalid branch '{}' for '{}'", + kernel_tree.branch(), + kernel_tree.path() + )); + } + + Ok(()) +} + +fn get_current_branch(shell: &dyn ShellTrait, kernel_tree: &KernelTree) -> Result { + let out = shell + .execute( + &ShellCommand::new("git") + .arg("-C") + .arg(kernel_tree.path()) + .args(["rev-parse", "--abbrev-ref", "HEAD"]), + ) + .map_err(|e| format!("failed to get current branch: {e}"))?; + + let mut branch = String::from_utf8_lossy(&out.stdout).to_string(); + branch.pop(); + Ok(branch) +} + +fn switch_to_branch( + shell: &dyn ShellTrait, + kernel_tree: &KernelTree, + branch: &str, +) -> Result<(), String> { + let out = shell + .execute( + &ShellCommand::new("git") + .arg("-C") + .arg(kernel_tree.path()) + .args(["switch", branch]), + ) + .map_err(|e| format!("failed to switch branch: {e}"))?; + + if !out.success { + return Err(format!( + "failed to switch to branch '{}': {}", + branch, + String::from_utf8_lossy(&out.stderr) + )); + } + + Ok(()) +} + +fn create_target_branch( + shell: &dyn ShellTrait, + kernel_tree: &KernelTree, + config: &ConfigSnapshot, +) -> Result { + switch_to_branch(shell, kernel_tree, kernel_tree.branch())?; + + let target_branch_name = format!( + "{}{}", + config.git_am_branch_prefix(), + Utc::now().format("%Y-%m-%d-%H-%M-%S") + ); + + let out = shell + .execute( + &ShellCommand::new("git") + .arg("-C") + .arg(kernel_tree.path()) + .args(["checkout", "-b", &target_branch_name]), + ) + .map_err(|e| format!("failed to create target branch: {e}"))?; + + if !out.success { + return Err(format!( + "failed to create branch '{}': {}", + target_branch_name, + String::from_utf8_lossy(&out.stderr) + )); + } + + Ok(target_branch_name) +} + +fn run_git_am( + request: &ApplyPatchsetRequest, + shell: &dyn ShellTrait, + kernel_tree: &KernelTree, + config: &ConfigSnapshot, +) -> Result<(), String> { + let mut git_am_cmd = ShellCommand::new("git") + .arg("-C") + .arg(kernel_tree.path()) + .args(["am", &request.patchset_path]); + for opt in config.git_am_options().split_whitespace() { + git_am_cmd = git_am_cmd.arg(opt); + } + + let out = shell + .execute(&git_am_cmd) + .map_err(|e| format!("failed to execute git-am: {e}"))?; + + if !out.success { + let _ = shell.execute( + &ShellCommand::new("git") + .arg("-C") + .arg(kernel_tree.path()) + .args(["am", "--abort"]), + ); + + return Err(String::from_utf8_lossy(&out.stderr).to_string()); + } + + Ok(()) +} + +#[cfg(test)] +mod tests { + use std::{ + collections::VecDeque, + sync::{Arc, Mutex}, + }; + + use crate::{ + config::{ConfigSnapshot, ConfigState}, + infrastructure::{ + file_system::MockFileSystemTrait, + shell::{MockShellTrait, ShellCommand, ShellOutput}, + }, + }; + + use super::*; + + const KERNEL_TREE_PATH: &str = "/kernel"; + const BASE_BRANCH: &str = "main"; + const PATCHSET_PATH: &str = "/tmp/patchset.mbx"; + + fn config() -> ConfigSnapshot { + serde_json::from_value::(serde_json::json!({ + "kernel_trees": { + "linux": { + "path": KERNEL_TREE_PATH, + "branch": BASE_BRANCH + } + }, + "target_kernel_tree": "linux", + "git_am_options": "--signoff --3way", + "git_am_branch_prefix": "patchset-" + })) + .expect("test config should deserialize") + .to_snapshot() + } + + fn config_without_target() -> ConfigSnapshot { + ConfigState::default().to_snapshot() + } + + fn request() -> ApplyPatchsetRequest { + ApplyPatchsetRequest { + patch_title: "[PATCH] test".to_string(), + patchset_path: PATCHSET_PATH.to_string(), + } + } + + fn output( + stdout: impl Into>, + stderr: impl Into>, + success: bool, + ) -> ShellOutput { + ShellOutput { + stdout: stdout.into(), + stderr: stderr.into(), + success, + } + } + + fn clean_fs() -> MockFileSystemTrait { + let mut fs = MockFileSystemTrait::new(); + fs.expect_is_dir() + .returning(|path| matches!(path.to_str(), Some("/kernel") | Some("/kernel/.git"))); + fs.expect_is_file().returning(|_| false); + fs + } + + fn shell_with_outputs( + outputs: Vec, + ) -> (MockShellTrait, Arc>>>) { + let calls = Arc::new(Mutex::new(Vec::new())); + let outputs = Arc::new(Mutex::new(VecDeque::from(outputs))); + let mut shell = MockShellTrait::new(); + let calls_for_execute = Arc::clone(&calls); + let outputs_for_execute = Arc::clone(&outputs); + shell.expect_execute().returning(move |cmd| { + calls_for_execute.lock().unwrap().push(command_parts(cmd)); + Ok(outputs_for_execute + .lock() + .unwrap() + .pop_front() + .expect("test should provide one output per shell command")) + }); + (shell, calls) + } + + fn command_parts(cmd: &ShellCommand) -> Vec { + let mut parts = vec![cmd.program.clone()]; + parts.extend(cmd.args.clone()); + parts + } + + fn command(parts: &[&str]) -> Vec { + parts.iter().map(|part| part.to_string()).collect() + } + + #[test] + fn apply_success_runs_expected_git_sequence() { + let fs = clean_fs(); + let (shell, calls) = shell_with_outputs(vec![ + output("", "", true), + output("", "", true), + output("feature\n", "", true), + output("", "", true), + output("", "", true), + output("", "", true), + output("", "", true), + ]); + + let result = apply_patchset(&request(), &fs, &shell, &config()).unwrap(); + + assert!(result.contains("Patchset '[PATCH] test' applied successfully")); + assert!(result.contains("Applied branch: 'patchset-")); + let calls = calls.lock().unwrap(); + assert_eq!( + &calls[0], + &command(&["git", "-C", KERNEL_TREE_PATH, "status", "--porcelain"]) + ); + assert_eq!( + &calls[1], + &command(&[ + "git", + "-C", + KERNEL_TREE_PATH, + "show-ref", + "--verify", + "--quiet", + "refs/heads/main" + ]) + ); + assert_eq!( + &calls[2], + &command(&[ + "git", + "-C", + KERNEL_TREE_PATH, + "rev-parse", + "--abbrev-ref", + "HEAD" + ]) + ); + assert_eq!( + &calls[3], + &command(&["git", "-C", KERNEL_TREE_PATH, "switch", BASE_BRANCH]) + ); + assert_eq!( + &calls[4][0..5], + command(&["git", "-C", KERNEL_TREE_PATH, "checkout", "-b"]).as_slice() + ); + assert!(calls[4][5].starts_with("patchset-")); + assert_eq!( + &calls[5], + &command(&[ + "git", + "-C", + KERNEL_TREE_PATH, + "am", + PATCHSET_PATH, + "--signoff", + "--3way" + ]) + ); + assert_eq!( + &calls[6], + &command(&["git", "-C", KERNEL_TREE_PATH, "switch", "feature"]) + ); + } + + #[test] + fn dirty_worktree_rejects_before_branch_creation() { + let fs = clean_fs(); + let (shell, calls) = shell_with_outputs(vec![output(" M file.rs\n", "", true)]); + + let result = apply_patchset(&request(), &fs, &shell, &config()).unwrap_err(); + + assert!(result.contains("there are staged and/or unstaged changes")); + assert_eq!(1, calls.lock().unwrap().len()); + } + + #[test] + fn missing_target_kernel_tree_rejects_before_shell_commands() { + let mut fs = MockFileSystemTrait::new(); + fs.expect_is_dir().times(0); + fs.expect_is_file().times(0); + let mut shell = MockShellTrait::new(); + shell.expect_execute().times(0); + + let result = apply_patchset(&request(), &fs, &shell, &config_without_target()).unwrap_err(); + + assert_eq!("target kernel tree unset", result); + } + + #[test] + fn invalid_configured_branch_rejects_before_branch_creation() { + let fs = clean_fs(); + let (shell, calls) = shell_with_outputs(vec![ + output("", "", true), + output("", "missing branch", false), + ]); + + let result = apply_patchset(&request(), &fs, &shell, &config()).unwrap_err(); + + assert!(result.contains("invalid branch 'main'")); + assert_eq!(2, calls.lock().unwrap().len()); + } + + #[test] + fn failed_git_am_aborts_and_switches_back() { + let fs = clean_fs(); + let (shell, calls) = shell_with_outputs(vec![ + output("", "", true), + output("", "", true), + output("feature\n", "", true), + output("", "", true), + output("", "", true), + output("", "apply failed", false), + output("", "", true), + output("", "", true), + ]); + + let result = apply_patchset(&request(), &fs, &shell, &config()).unwrap_err(); + + assert!(result.contains("`git am` failed")); + assert!(result.contains("feature")); + assert!(result.contains("apply failed")); + let calls = calls.lock().unwrap(); + assert_eq!( + &calls[6], + &command(&["git", "-C", KERNEL_TREE_PATH, "am", "--abort"]) + ); + assert_eq!( + &calls[7], + &command(&["git", "-C", KERNEL_TREE_PATH, "switch", "feature"]) + ); + } +} diff --git a/src/app/actions/mod.rs b/src/app/actions/mod.rs new file mode 100644 index 0000000..43cbf5c --- /dev/null +++ b/src/app/actions/mod.rs @@ -0,0 +1 @@ +pub(crate) mod apply; diff --git a/src/app/mod.rs b/src/app/mod.rs index 8ee2cfe..edae486 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -9,6 +9,7 @@ //! [`AppActor`](crate::app::actor::AppActor) into [`crate::app::flows`]; //! presentation data crosses the UI boundary only through [`AppViewModel`] via //! [`App::present`]. +pub(crate) mod actions; pub mod actor; pub(crate) mod dependencies; pub mod errors; @@ -31,6 +32,7 @@ use tracing::{debug, event, info, warn, Level}; use std::{path::PathBuf, str}; use crate::{ + app::actions::apply::{apply_patchset, ApplyPatchsetRequest}, config::{ConfigHandle, ConfigSnapshot}, infrastructure::{ file_system::FileSystemTrait, @@ -454,17 +456,22 @@ impl App { .get(&PatchsetAction::Apply) { debug!("applying patchset via git-am"); - let popup = match self + let details = 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, - ) { + .expect("invariant: details must be loaded before applying patchset"); + let request = ApplyPatchsetRequest { + patch_title: details.representative_patch.title().clone(), + patchset_path: details.patchset_path.clone(), + }; + let popup = match apply_patchset( + &request, + &*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/screens/details_actions.rs b/src/app/screens/details_actions.rs index a7aa6fd..d07f1d2 100644 --- a/src/app/screens/details_actions.rs +++ b/src/app/screens/details_actions.rs @@ -1,15 +1,8 @@ use std::collections::{HashMap, HashSet}; -use std::path::Path; use ansi_to_tui::IntoText; -use chrono::Utc; use crate::{ - config::{ConfigSnapshot, KernelTree}, - infrastructure::{ - file_system::FileSystemTrait, - shell::{ShellCommand, ShellTrait}, - }, lore::{ application::dto::PatchsetDetails, domain::patch::{Author, Patch}, @@ -224,259 +217,6 @@ impl PatchsetDetailsState { pub fn actions_require_user_io(&self) -> bool { self.patches_to_reply.contains(&true) } - - /// Checks if there is a `target_kernel_tree` and if it is in `Config::kernel_trees` and if - /// that kernel tree is a valid git directory. - /// - /// Returns the a valid `KernelTree` or a `String` with the error message on failure. - fn validate_kernel_tree<'a>( - &self, - fs: &dyn FileSystemTrait, - config: &'a ConfigSnapshot, - ) -> Result<&'a KernelTree, String> { - let kernel_tree_id = if let Some(target) = config.target_kernel_tree() { - target - } else { - return Err("target kernel tree unset".to_string()); - }; - - let kernel_tree = if let Some(tree) = config.get_kernel_tree(kernel_tree_id) { - tree - } else { - return Err(format!("invalid target kernel tree '{kernel_tree_id}'")); - }; - - let kernel_tree_path = Path::new(kernel_tree.path()); - if !fs.is_dir(kernel_tree_path) { - return Err(format!("{} isn't a directory", kernel_tree.path())); - } else if !fs.is_dir(&kernel_tree_path.join(".git")) { - return Err(format!("{} isn't a git repository", kernel_tree.path())); - } - - Ok(kernel_tree) - } - - // Ensures the kernel directory is not currently in another git operation, - // that it does not have unstaged or uncommited changes, and that the base branch - // is valid. - // - // Returns `()` on success and a `String` with an error message on failure. - fn check_git_state( - &self, - fs: &dyn FileSystemTrait, - shell: &dyn ShellTrait, - kernel_tree: &KernelTree, - ) -> Result<(), String> { - let kernel_tree_path = Path::new(kernel_tree.path()); - - if fs.is_dir(&kernel_tree_path.join(".git/rebase-merge")) { - return Err( - "rebase in progress. \nrun `git rebase --abort` before continuing".to_string(), - ); - } else if fs.is_file(&kernel_tree_path.join(".git/MERGE_HEAD")) { - return Err( - "merge in progress. \nrun `git merge --abort` before continuing".to_string(), - ); - } else if fs.is_file(&kernel_tree_path.join(".git/BISECT_LOG")) { - return Err( - "bisect in progress. \nrun `git bisect reset` before continuing".to_string(), - ); - } else if fs.is_dir(&kernel_tree_path.join(".git/rebase-apply")) { - return Err( - "`git am` already in progress. \nrun `git am --abort` before continuing" - .to_string(), - ); - } - - let status_out = shell - .execute( - &ShellCommand::new("git") - .arg("-C") - .arg(kernel_tree.path()) - .args(["status", "--porcelain"]), - ) - .map_err(|e| format!("failed to check git status {e}"))?; - - let status_output = String::from_utf8_lossy(&status_out.stdout); - if !status_output.is_empty() { - return Err(format!( - "there are staged and/or unstaged changes\n{status_output}" - )); - } - - let show_ref_out = shell - .execute( - &ShellCommand::new("git") - .arg("-C") - .arg(kernel_tree.path()) - .args(["show-ref", "--verify", "--quiet"]) - .arg(format!("refs/heads/{}", kernel_tree.branch())), - ) - .map_err(|e| format!("failed to verify branch: {e}"))?; - - if !show_ref_out.success { - return Err(format!( - "invalid branch '{}' for '{}'", - kernel_tree.branch(), - kernel_tree.path() - )); - } - - Ok(()) - } - - /// Get the current branch of the supplied kernel tree - /// - /// Returns the branch name as a `String` or a `String` with the error message on failure - fn get_current_branch( - &self, - shell: &dyn ShellTrait, - kernel_tree: &KernelTree, - ) -> Result { - let out = shell - .execute( - &ShellCommand::new("git") - .arg("-C") - .arg(kernel_tree.path()) - .args(["rev-parse", "--abbrev-ref", "HEAD"]), - ) - .map_err(|e| format!("failed to get current branch: {e}"))?; - - let mut branch = String::from_utf8_lossy(&out.stdout).to_string(); - branch.pop(); - Ok(branch) - } - - /// Switch the supplied kernel tree to the supplied branch, if it exists. - /// - /// Returns `()` on sucess and a `String` with the error message on failure. - fn switch_to_branch( - &self, - shell: &dyn ShellTrait, - kernel_tree: &KernelTree, - branch: &str, - ) -> Result<(), String> { - let out = shell - .execute( - &ShellCommand::new("git") - .arg("-C") - .arg(kernel_tree.path()) - .args(["switch", branch]), - ) - .map_err(|e| format!("failed to switch branch: {e}"))?; - - if !out.success { - return Err(format!( - "failed to switch to branch '{}': {}", - branch, - String::from_utf8_lossy(&out.stderr) - )); - } - - Ok(()) - } - - /// Create a new branch suffixed with the current timestamp. - /// - /// Returns a `String` with the branch name on success or the - /// error message on failure. - fn create_target_branch( - &self, - shell: &dyn ShellTrait, - kernel_tree: &KernelTree, - config: &ConfigSnapshot, - ) -> Result { - self.switch_to_branch(shell, kernel_tree, kernel_tree.branch())?; - - let target_branch_name = format!( - "{}{}", - config.git_am_branch_prefix(), - Utc::now().format("%Y-%m-%d-%H-%M-%S") - ); - - let out = shell - .execute( - &ShellCommand::new("git") - .arg("-C") - .arg(kernel_tree.path()) - .args(["checkout", "-b", &target_branch_name]), - ) - .map_err(|e| format!("failed to create target branch: {e}"))?; - - if !out.success { - return Err(format!( - "failed to create branch '{}': {}", - target_branch_name, - String::from_utf8_lossy(&out.stderr) - )); - } - - Ok(target_branch_name) - } - - /// Apply the selected patchset on the given `kernel_tree` with arguments from `Config` - /// - /// Returns `()` on sucess and a `String` containing the error message on failure. - fn run_git_am( - &self, - shell: &dyn ShellTrait, - kernel_tree: &KernelTree, - config: &ConfigSnapshot, - ) -> Result<(), String> { - let mut git_am_cmd = ShellCommand::new("git") - .arg("-C") - .arg(kernel_tree.path()) - .args(["am", &self.patchset_path]); - for opt in config.git_am_options().split_whitespace() { - git_am_cmd = git_am_cmd.arg(opt); - } - - let out = shell - .execute(&git_am_cmd) - .map_err(|e| format!("failed to execute git-am: {e}"))?; - - if !out.success { - let _ = shell.execute( - &ShellCommand::new("git") - .arg("-C") - .arg(kernel_tree.path()) - .args(["am", "--abort"]), - ); - - return Err(String::from_utf8_lossy(&out.stderr).to_string()); - } - - Ok(()) - } - - /// Try to apply the patchset to a target kernel tree and returns a `String` - /// informing if the apply succeeded or failed and why. - /// - /// Returns a `Result` containing either the success or the error message. - /// # TODO: - /// - Add unit tests - pub fn apply_patchset( - &self, - fs: &dyn FileSystemTrait, - shell: &dyn ShellTrait, - config: &ConfigSnapshot, - ) -> Result { - let kernel_tree = self.validate_kernel_tree(fs, config)?; - self.check_git_state(fs, shell, kernel_tree)?; - - let original_branch = self.get_current_branch(shell, kernel_tree)?; - let target_branch = self.create_target_branch(shell, kernel_tree, config)?; - - let git_am_result = self.run_git_am(shell, kernel_tree, config); - self.switch_to_branch(shell, kernel_tree, &original_branch)?; - - match git_am_result { - Ok(_) => { - Ok(format!(" Patchset '{}' applied successfully!\n\n - Kernel Tree: '{}'\n\n - Base Branch: '{}'\n\n - Applied branch: '{}'", self.representative_patch.title(), kernel_tree.path(), kernel_tree.branch(), &target_branch)) - }, - Err(e) => Err(format!( "`git am` failed\n{}{}", &original_branch, e)) - } - } } #[cfg(test)] From 94535ed153d9abd16e2bd4e04c94f26c35d9cd84 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 9 Jul 2026 15:44:24 -0300 Subject: [PATCH 09/18] refactor(app): extract reviewed-by reply action service This commit moves reviewed-by reply preparation and interactive command execution into a focused application action service. App retains reviewed-state persistence and UI reset while the reply workflow lives behind a named action boundary. This commit is part of the architecture's refactoring phase 15. Signed-off-by: lorenzoberts --- src/app/actions/mod.rs | 1 + src/app/actions/reviewed_reply.rs | 209 ++++++++++++++++++++++++++++++ src/app/mod.rs | 84 ++++-------- 3 files changed, 232 insertions(+), 62 deletions(-) create mode 100644 src/app/actions/reviewed_reply.rs diff --git a/src/app/actions/mod.rs b/src/app/actions/mod.rs index 43cbf5c..1bbb181 100644 --- a/src/app/actions/mod.rs +++ b/src/app/actions/mod.rs @@ -1 +1,2 @@ pub(crate) mod apply; +pub(crate) mod reviewed_reply; diff --git a/src/app/actions/reviewed_reply.rs b/src/app/actions/reviewed_reply.rs new file mode 100644 index 0000000..51bba6e --- /dev/null +++ b/src/app/actions/reviewed_reply.rs @@ -0,0 +1,209 @@ +use std::{collections::HashSet, path::PathBuf, str}; + +use color_eyre::{eyre::eyre, Result}; + +use crate::{ + infrastructure::shell::{ShellCommand, ShellTrait}, + lore::application::handle::LoreApiHandle, +}; + +pub(crate) struct ReviewedReplyRequest { + pub raw_patches: Vec, + pub patches_to_reply: Vec, + pub successful_indexes: HashSet, + pub git_send_email_options: String, +} + +pub(crate) enum ReviewedReplyResult { + NoAction { successful_indexes: HashSet }, + MissingGitIdentity { successful_indexes: HashSet }, + Completed { successful_indexes: HashSet }, +} + +impl ReviewedReplyResult { + pub(crate) fn into_successful_indexes(self) -> HashSet { + match self { + ReviewedReplyResult::NoAction { successful_indexes } + | ReviewedReplyResult::MissingGitIdentity { successful_indexes } + | ReviewedReplyResult::Completed { successful_indexes } => successful_indexes, + } + } +} + +pub(crate) async fn execute_reviewed_reply( + request: ReviewedReplyRequest, + lore_api: &LoreApiHandle, + shell: &dyn ShellTrait, +) -> Result { + if !request.patches_to_reply.contains(&true) { + return Ok(ReviewedReplyResult::NoAction { + successful_indexes: request.successful_indexes, + }); + } + + let (git_user_name, git_user_email) = lore_api + .get_git_signature(String::new()) + .await + .map_err(|e| eyre!("{e:#?}"))?; + + let mut successful_indexes = request.successful_indexes; + let Some(git_signature) = git_signature(git_user_name, git_user_email) else { + println!("`git config user.name` or `git config user.email` not set\nAborting..."); + return Ok(ReviewedReplyResult::MissingGitIdentity { successful_indexes }); + }; + + let mktemp_cmd = ShellCommand::new("mktemp").arg("--directory"); + let tmp_out = shell + .execute(&mktemp_cmd) + .map_err(|e| eyre!("failed to create temp directory: {}", e))?; + let tmp_dir_str = str::from_utf8(&tmp_out.stdout) + .map_err(|e| eyre!("invalid utf-8 in temp dir path: {}", e))? + .trim() + .to_string(); + let tmp_dir = PathBuf::from(tmp_dir_str); + + let git_reply_commands = lore_api + .prepare_reply_commands( + tmp_dir, + "all".to_string(), + request.raw_patches, + request.patches_to_reply.clone(), + git_signature, + request.git_send_email_options, + ) + .await + .map_err(|e| eyre!("{e:#?}"))?; + + record_successful_reply_indexes( + shell, + &mut successful_indexes, + &request.patches_to_reply, + git_reply_commands, + ); + + Ok(ReviewedReplyResult::Completed { successful_indexes }) +} + +fn record_successful_reply_indexes( + shell: &dyn ShellTrait, + successful_indexes: &mut HashSet, + patches_to_reply: &[bool], + git_reply_commands: Vec, +) { + let reply_indexes: Vec = selected_reply_indexes(patches_to_reply); + for (i, command) in git_reply_commands.into_iter().enumerate() { + let success = shell.spawn_interactive(&command).unwrap_or(false); + if success { + successful_indexes.insert(reply_indexes[i]); + } + } +} + +fn selected_reply_indexes(patches_to_reply: &[bool]) -> Vec { + patches_to_reply + .iter() + .enumerate() + .filter_map(|(i, &val)| if val { Some(i) } else { None }) + .collect() +} + +fn git_signature(git_user_name: String, git_user_email: String) -> Option { + if git_user_name.is_empty() || git_user_email.is_empty() { + None + } else { + Some(format!("{git_user_name} <{git_user_email}>")) + } +} + +#[cfg(test)] +mod tests { + use std::{collections::HashSet, io}; + + use crate::infrastructure::shell::{MockShellTrait, ShellError}; + + use super::*; + + fn command(name: &str) -> ShellCommand { + ShellCommand::new("git").arg("send-email").arg(name) + } + + #[test] + fn selected_reply_indexes_preserves_original_patch_indexes() { + let indexes = selected_reply_indexes(&[false, true, false, true]); + + assert_eq!(vec![1, 3], indexes); + } + + #[test] + fn git_signature_requires_name_and_email() { + assert_eq!( + Some("User ".to_string()), + git_signature("User".to_string(), "user@example.com".to_string()) + ); + assert_eq!( + None, + git_signature(String::new(), "user@example.com".to_string()) + ); + assert_eq!(None, git_signature("User".to_string(), String::new())); + } + + #[test] + fn record_successful_reply_indexes_records_only_successful_commands() { + let mut shell = MockShellTrait::new(); + shell + .expect_spawn_interactive() + .times(2) + .returning(|cmd| Ok(cmd.args.last().is_some_and(|arg| arg == "first"))); + let mut successful_indexes = HashSet::from([0]); + + record_successful_reply_indexes( + &shell, + &mut successful_indexes, + &[false, true, false, true], + vec![command("first"), command("second")], + ); + + assert_eq!(HashSet::from([0, 1]), successful_indexes); + } + + #[test] + fn record_successful_reply_indexes_treats_shell_error_as_failure() { + let mut shell = MockShellTrait::new(); + shell.expect_spawn_interactive().times(1).returning(|_| { + Err(ShellError::IoError(io::Error::new( + io::ErrorKind::Other, + "failed", + ))) + }); + let mut successful_indexes = HashSet::new(); + + record_successful_reply_indexes( + &shell, + &mut successful_indexes, + &[true], + vec![command("first")], + ); + + assert!(successful_indexes.is_empty()); + } + + #[test] + fn reviewed_reply_result_returns_successful_indexes_for_each_status() { + let no_action = ReviewedReplyResult::NoAction { + successful_indexes: HashSet::from([1]), + }; + let missing_identity = ReviewedReplyResult::MissingGitIdentity { + successful_indexes: HashSet::from([2]), + }; + let completed = ReviewedReplyResult::Completed { + successful_indexes: HashSet::from([3]), + }; + + assert_eq!(HashSet::from([1]), no_action.into_successful_indexes()); + assert_eq!( + HashSet::from([2]), + missing_identity.into_successful_indexes() + ); + assert_eq!(HashSet::from([3]), completed.into_successful_indexes()); + } +} diff --git a/src/app/mod.rs b/src/app/mod.rs index edae486..b490b01 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -29,15 +29,17 @@ use color_eyre::{ }; use tracing::{debug, event, info, warn, Level}; -use std::{path::PathBuf, str}; - use crate::{ - app::actions::apply::{apply_patchset, ApplyPatchsetRequest}, + app::actions::{ + apply::{apply_patchset, ApplyPatchsetRequest}, + reviewed_reply::{ + execute_reviewed_reply as execute_reviewed_reply_action, ReviewedReplyRequest, + }, + }, config::{ConfigHandle, ConfigSnapshot}, infrastructure::{ - file_system::FileSystemTrait, - monitoring::logging::garbage_collector::collect_garbage, - shell::{ShellCommand, ShellTrait}, + file_system::FileSystemTrait, monitoring::logging::garbage_collector::collect_garbage, + shell::ShellTrait, }, lore::{ application::{ @@ -359,70 +361,28 @@ impl App { msg_id = representative_patch.message_id().href, "executing reviewed-by reply" ); - let mut successful_indexes = self + let successful_indexes = self .state .user_state .reviewed_patchsets .remove(&representative_patch.message_id().href) .unwrap_or_default(); - - let (git_user_name, git_user_email) = self - .services - .lore_api - .get_git_signature(String::new()) - .await - .map_err(|e| eyre!("{e:#?}"))?; - - if git_user_name.is_empty() || git_user_email.is_empty() { - println!("`git config user.name` or `git config user.email` not set\nAborting..."); - } else { - let mktemp_cmd = ShellCommand::new("mktemp").arg("--directory"); - let tmp_out = self - .services - .shell - .execute(&mktemp_cmd) - .map_err(|e| eyre!("failed to create temp directory: {}", e))?; - let tmp_dir_str = str::from_utf8(&tmp_out.stdout) - .map_err(|e| eyre!("invalid utf-8 in temp dir path: {}", e))? - .trim() - .to_string(); - let tmp_dir = PathBuf::from(tmp_dir_str); - - let git_signature = format!("{git_user_name} <{git_user_email}>"); - let git_reply_commands = self - .services - .lore_api - .prepare_reply_commands( - tmp_dir, - "all".to_string(), - raw_patches, - patches_to_reply.clone(), - git_signature, - self.state.config.git_send_email_options().to_string(), - ) - .await - .map_err(|e| eyre!("{e:#?}"))?; - - let reply_indexes: Vec = patches_to_reply - .iter() - .enumerate() - .filter_map(|(i, &val)| if val { Some(i) } else { None }) - .collect(); - for (i, command) in git_reply_commands.into_iter().enumerate() { - let success = self - .services - .shell - .spawn_interactive(&command) - .unwrap_or(false); - if success { - successful_indexes.insert(reply_indexes[i]); - } - } - } + let request = ReviewedReplyRequest { + raw_patches, + patches_to_reply, + successful_indexes, + git_send_email_options: self.state.config.git_send_email_options().to_string(), + }; + let result = execute_reviewed_reply_action( + request, + &self.services.lore_api, + &*self.services.shell, + ) + .await?; self.state.user_state.reviewed_patchsets.insert( representative_patch.message_id().href.clone(), - successful_indexes, + result.into_successful_indexes(), ); self.services From d1af06c56c5e5f4d9ba490ae752d596613c778be Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 9 Jul 2026 15:47:10 -0300 Subject: [PATCH 10/18] refactor(app): simplify patchset action consolidation This commit clarifies App's patchset action orchestration by extracting request builders and action-selection checks around the apply and reviewed-by reply services. Remaining App-side mapping logic is kept small and testable at the orchestration layer. This commit is part of the architecture's refactoring phase 15. Signed-off-by: lorenzoberts --- src/app/mod.rs | 174 +++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 138 insertions(+), 36 deletions(-) diff --git a/src/app/mod.rs b/src/app/mod.rs index b490b01..86afe64 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -351,28 +351,20 @@ impl App { .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" - ); + if patchset_action_selected(details, &PatchsetAction::ReplyWithReviewedBy) { + let message_id = details.representative_patch.message_id().href.clone(); + debug!(msg_id = message_id, "executing reviewed-by reply"); let successful_indexes = self .state .user_state .reviewed_patchsets - .remove(&representative_patch.message_id().href) + .remove(&message_id) .unwrap_or_default(); - let request = ReviewedReplyRequest { - raw_patches, - patches_to_reply, + let request = reviewed_reply_request( + details, successful_indexes, - git_send_email_options: self.state.config.git_send_email_options().to_string(), - }; + self.state.config.git_send_email_options().to_string(), + ); let result = execute_reviewed_reply_action( request, &self.services.lore_api, @@ -380,10 +372,10 @@ impl App { ) .await?; - self.state.user_state.reviewed_patchsets.insert( - representative_patch.message_id().href.clone(), - result.into_successful_indexes(), - ); + self.state + .user_state + .reviewed_patchsets + .insert(message_id.clone(), result.into_successful_indexes()); self.services .lore_api @@ -392,7 +384,7 @@ impl App { .map_err(|e| eyre!("{e:#?}"))?; info!( - msg_id = representative_patch.message_id().href, + msg_id = message_id, "reviewed-by reply sent and state persisted" ); self.state @@ -406,26 +398,16 @@ impl App { } fn execute_apply_patchset(&mut self) { - if let Some(true) = self + let details = self .state .lore .details .as_ref() - .expect("invariant: details must be loaded before executing apply patchset") - .patchset_actions - .get(&PatchsetAction::Apply) - { + .expect("invariant: details must be loaded before executing apply patchset"); + + if patchset_action_selected(details, &PatchsetAction::Apply) { debug!("applying patchset via git-am"); - let details = self - .state - .lore - .details - .as_ref() - .expect("invariant: details must be loaded before applying patchset"); - let request = ApplyPatchsetRequest { - patch_title: details.representative_patch.title().clone(), - patchset_path: details.patchset_path.clone(), - }; + let request = apply_patchset_request(details); let popup = match apply_patchset( &request, &*self.services.fs, @@ -485,3 +467,123 @@ impl App { view_model::project_state(&self.state) } } + +fn patchset_action_selected(details: &PatchsetDetailsState, action: &PatchsetAction) -> bool { + matches!(details.patchset_actions.get(action), Some(true)) +} + +fn reviewed_reply_request( + details: &PatchsetDetailsState, + successful_indexes: std::collections::HashSet, + git_send_email_options: String, +) -> ReviewedReplyRequest { + ReviewedReplyRequest { + raw_patches: details.raw_patches.clone(), + patches_to_reply: details.patches_to_reply.clone(), + successful_indexes, + git_send_email_options, + } +} + +fn apply_patchset_request(details: &PatchsetDetailsState) -> ApplyPatchsetRequest { + ApplyPatchsetRequest { + patch_title: details.representative_patch.title().clone(), + patchset_path: details.patchset_path.clone(), + } +} + +#[cfg(test)] +mod tests { + use std::collections::{HashMap, HashSet}; + + use serde_xml_rs::from_str; + + use super::*; + + fn test_patch() -> Patch { + from_str( + r#" + + + Foo Bar + foo@bar.foo.bar + + [PATCH 1/1] test patch + 2024-07-06T19:15:48Z + + urn:uuid:123-abcd-1f2a3b + + + "#, + ) + .expect("test patch XML should deserialize") + } + + fn details_state() -> PatchsetDetailsState { + PatchsetDetailsState { + representative_patch: test_patch(), + raw_patches: vec!["raw patch 0".to_string(), "raw patch 1".to_string()], + patches_preview: vec!["preview 0".to_string(), "preview 1".to_string()], + has_cover_letter: false, + patches_to_reply: vec![false, true], + patchset_path: "/tmp/patchset.mbx".to_string(), + preview_index: 0, + preview_scroll_offset: 0, + preview_pan: 0, + preview_fullscreen: false, + patchset_actions: HashMap::from([ + (PatchsetAction::Bookmark, false), + (PatchsetAction::ReplyWithReviewedBy, true), + (PatchsetAction::Apply, true), + ]), + reviewed_by: vec![HashSet::new(), HashSet::new()], + tested_by: vec![HashSet::new(), HashSet::new()], + acked_by: vec![HashSet::new(), HashSet::new()], + last_screen: CurrentScreen::LatestPatchsets, + } + } + + #[test] + fn patchset_action_selected_reads_action_map() { + let mut details = details_state(); + + assert!(patchset_action_selected(&details, &PatchsetAction::Apply)); + assert!(patchset_action_selected( + &details, + &PatchsetAction::ReplyWithReviewedBy + )); + + details + .patchset_actions + .insert(PatchsetAction::Apply, false); + + assert!(!patchset_action_selected(&details, &PatchsetAction::Apply)); + } + + #[test] + fn reviewed_reply_request_copies_reply_inputs() { + let details = details_state(); + let request = reviewed_reply_request( + &details, + HashSet::from([4usize]), + "--dry-run --suppress-cc=all".to_string(), + ); + + assert_eq!(details.raw_patches, request.raw_patches); + assert_eq!(details.patches_to_reply, request.patches_to_reply); + assert_eq!(HashSet::from([4]), request.successful_indexes); + assert_eq!( + "--dry-run --suppress-cc=all", + request.git_send_email_options + ); + } + + #[test] + fn apply_patchset_request_copies_apply_inputs() { + let details = details_state(); + let request = apply_patchset_request(&details); + + assert_eq!("[PATCH 1/1] test patch", request.patch_title); + assert_eq!("/tmp/patchset.mbx", request.patchset_path); + } +} From 7bd158a3705adb9fd6b4d9d8e678512bda714353 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 9 Jul 2026 15:52:08 -0300 Subject: [PATCH 11/18] refactor(app): group patchset actions behind service This commit introduces a PatchsetActionService facade over the apply and reviewed-by reply action modules. App now depends on a single named action boundary instead of calling loose execution functions directly. This commit is part of the architecture's refactoring phase 15. Signed-off-by: lorenzoberts --- src/app/actions/mod.rs | 46 ++++++++++++++++++++++++++++++++++++++++++ src/app/mod.rs | 23 +++++++++------------ 2 files changed, 56 insertions(+), 13 deletions(-) diff --git a/src/app/actions/mod.rs b/src/app/actions/mod.rs index 1bbb181..82d0967 100644 --- a/src/app/actions/mod.rs +++ b/src/app/actions/mod.rs @@ -1,2 +1,48 @@ pub(crate) mod apply; pub(crate) mod reviewed_reply; + +use color_eyre::Result; + +use crate::{ + config::ConfigSnapshot, + infrastructure::{file_system::FileSystemTrait, shell::ShellTrait}, + lore::application::handle::LoreApiHandle, +}; + +use apply::ApplyPatchsetRequest; +use reviewed_reply::{ReviewedReplyRequest, ReviewedReplyResult}; + +pub(crate) struct PatchsetActionService<'a> { + fs: &'a dyn FileSystemTrait, + shell: &'a dyn ShellTrait, + lore_api: &'a LoreApiHandle, +} + +impl<'a> PatchsetActionService<'a> { + pub(crate) fn new( + fs: &'a dyn FileSystemTrait, + shell: &'a dyn ShellTrait, + lore_api: &'a LoreApiHandle, + ) -> Self { + Self { + fs, + shell, + lore_api, + } + } + + pub(crate) fn apply_patchset( + &self, + request: &ApplyPatchsetRequest, + config: &ConfigSnapshot, + ) -> Result { + apply::apply_patchset(request, self.fs, self.shell, config) + } + + pub(crate) async fn execute_reviewed_reply( + &self, + request: ReviewedReplyRequest, + ) -> Result { + reviewed_reply::execute_reviewed_reply(request, self.lore_api, self.shell).await + } +} diff --git a/src/app/mod.rs b/src/app/mod.rs index 86afe64..a5bee78 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -31,10 +31,7 @@ use tracing::{debug, event, info, warn, Level}; use crate::{ app::actions::{ - apply::{apply_patchset, ApplyPatchsetRequest}, - reviewed_reply::{ - execute_reviewed_reply as execute_reviewed_reply_action, ReviewedReplyRequest, - }, + apply::ApplyPatchsetRequest, reviewed_reply::ReviewedReplyRequest, PatchsetActionService, }, config::{ConfigHandle, ConfigSnapshot}, infrastructure::{ @@ -365,12 +362,12 @@ impl App { successful_indexes, self.state.config.git_send_email_options().to_string(), ); - let result = execute_reviewed_reply_action( - request, - &self.services.lore_api, + let action_service = PatchsetActionService::new( + &*self.services.fs, &*self.services.shell, - ) - .await?; + &self.services.lore_api, + ); + let result = action_service.execute_reviewed_reply(request).await?; self.state .user_state @@ -408,12 +405,12 @@ impl App { if patchset_action_selected(details, &PatchsetAction::Apply) { debug!("applying patchset via git-am"); let request = apply_patchset_request(details); - let popup = match apply_patchset( - &request, + let action_service = PatchsetActionService::new( &*self.services.fs, &*self.services.shell, - &self.state.config, - ) { + &self.services.lore_api, + ); + let popup = match action_service.apply_patchset(&request, &self.state.config) { Ok(msg) => popup::AppPopup::info("Patchset Apply Success", msg), Err(msg) => popup::AppPopup::info("Patchset Apply Fail", msg), }; From 6563353b7ffbe6577baf2e9efa852eb978a3ace1 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 9 Jul 2026 16:07:37 -0300 Subject: [PATCH 12/18] test(app): add integration test module shell This commit introduces a dedicated app integration test module for behavior-oriented flow coverage. The module is wired into the app crate without moving existing unit tests, establishing a compilable home for the follow-up helper and flow tests. This commit is part of the architecture's refactoring phase 15. Signed-off-by: lorenzoberts --- src/app/integration_tests/mod.rs | 6 ++++++ src/app/mod.rs | 3 +++ 2 files changed, 9 insertions(+) create mode 100644 src/app/integration_tests/mod.rs diff --git a/src/app/integration_tests/mod.rs b/src/app/integration_tests/mod.rs new file mode 100644 index 0000000..44c708e --- /dev/null +++ b/src/app/integration_tests/mod.rs @@ -0,0 +1,6 @@ +#[test] +fn integration_test_module_is_wired() { + // Keeps the module compiling before the shared harness and behavior tests + // are introduced in follow-up slices. + assert!(true); +} diff --git a/src/app/mod.rs b/src/app/mod.rs index a5bee78..39edf71 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -23,6 +23,9 @@ pub mod state; pub mod updates; pub mod view_model; +#[cfg(test)] +mod integration_tests; + use color_eyre::{ eyre::{bail, eyre}, Result, From 0cd1008bc3df307b7ef53c0ff09d724e15de4929 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 9 Jul 2026 16:11:10 -0300 Subject: [PATCH 13/18] test(app): add integration test helpers This commit adds shared helpers for building minimal App state, sample Lore and render data, dummy actor handles, and a fake loading indicator. The helpers provide a reusable foundation for behavior-oriented app flow tests without depending on terminal loading frames. This commit is part of the architecture's refactoring phase 15. Signed-off-by: lorenzoberts --- .../integration_tests/helpers/app_harness.rs | 59 +++++++++++++++++++ src/app/integration_tests/helpers/loading.rs | 20 +++++++ src/app/integration_tests/helpers/lore.rs | 52 ++++++++++++++++ src/app/integration_tests/helpers/mod.rs | 4 ++ src/app/integration_tests/helpers/render.rs | 7 +++ src/app/integration_tests/mod.rs | 45 ++++++++++++-- 6 files changed, 183 insertions(+), 4 deletions(-) create mode 100644 src/app/integration_tests/helpers/app_harness.rs create mode 100644 src/app/integration_tests/helpers/loading.rs create mode 100644 src/app/integration_tests/helpers/lore.rs create mode 100644 src/app/integration_tests/helpers/mod.rs create mode 100644 src/app/integration_tests/helpers/render.rs diff --git a/src/app/integration_tests/helpers/app_harness.rs b/src/app/integration_tests/helpers/app_harness.rs new file mode 100644 index 0000000..5141023 --- /dev/null +++ b/src/app/integration_tests/helpers/app_harness.rs @@ -0,0 +1,59 @@ +use tokio::sync::mpsc; + +use crate::{ + app::App, + config::{ConfigHandle, ConfigState}, + infrastructure::{file_system::MockFileSystemTrait, shell::MockShellTrait}, + lore::application::{cache::BootstrapLoreData, handle::LoreApiHandle}, + render::handle::RenderHandle, +}; + +use super::lore::sample_mailing_list; + +pub(crate) struct AppHarness { + pub(crate) app: App, +} + +impl AppHarness { + pub(crate) fn new() -> Self { + Self { + app: minimal_app(), + } + } +} + +pub(crate) fn minimal_app() -> App { + App::new( + ConfigState::default().to_snapshot(), + dummy_config_handle(), + bootstrap_data(), + Box::new(MockFileSystemTrait::new()), + Box::new(MockShellTrait::new()), + dummy_lore_handle(), + dummy_render_handle(), + ) + .expect("minimal app should build") +} + +pub(crate) fn dummy_config_handle() -> ConfigHandle { + let (tx, _rx) = mpsc::channel(1); + ConfigHandle::new(tx) +} + +pub(crate) fn dummy_lore_handle() -> LoreApiHandle { + let (tx, _rx) = mpsc::channel(1); + LoreApiHandle::new(tx) +} + +pub(crate) fn dummy_render_handle() -> RenderHandle { + let (tx, _rx) = mpsc::channel(1); + RenderHandle::new(tx) +} + +fn bootstrap_data() -> BootstrapLoreData { + BootstrapLoreData { + mailing_lists: vec![sample_mailing_list()], + bookmarks: vec![], + reviewed: Default::default(), + } +} diff --git a/src/app/integration_tests/helpers/loading.rs b/src/app/integration_tests/helpers/loading.rs new file mode 100644 index 0000000..edf4607 --- /dev/null +++ b/src/app/integration_tests/helpers/loading.rs @@ -0,0 +1,20 @@ +use color_eyre::Result; + +use crate::app::loading::LoadingIndicator; + +#[derive(Default)] +pub(crate) struct FakeLoadingIndicator { + pub(crate) starts: Vec, + pub(crate) stop_count: usize, +} + +impl LoadingIndicator for FakeLoadingIndicator { + fn start(&mut self, title: String) { + self.starts.push(title); + } + + fn stop(&mut self) -> Result<()> { + self.stop_count += 1; + Ok(()) + } +} diff --git a/src/app/integration_tests/helpers/lore.rs b/src/app/integration_tests/helpers/lore.rs new file mode 100644 index 0000000..4c8023e --- /dev/null +++ b/src/app/integration_tests/helpers/lore.rs @@ -0,0 +1,52 @@ +use std::collections::HashSet; + +use serde_xml_rs::from_str; + +use crate::lore::{ + application::dto::{PatchTagSummary, PatchsetDetails}, + domain::{mailing_list::MailingList, patch::Patch}, +}; + +pub(crate) fn sample_mailing_list() -> MailingList { + MailingList::new("test-list", "Test list") +} + +pub(crate) fn sample_patch() -> Patch { + from_str( + r#" + + + Foo Bar + foo@bar.example + + [PATCH 1/1] test patch + 2024-07-06T19:15:48Z + + urn:uuid:123-abcd-1f2a3b + + + "#, + ) + .expect("sample patch XML should deserialize") +} + +pub(crate) fn sample_patchset_details() -> PatchsetDetails { + PatchsetDetails { + patchset_path: "/tmp/patchset.mbx".to_string(), + raw_patches: vec![sample_raw_patch()], + tag_summary: vec![empty_tag_summary()], + } +} + +pub(crate) fn sample_raw_patch() -> String { + "Subject: [PATCH] test\n\nBody\n---\n file.txt | 1 +\n 1 file changed, 1 insertion(+)\n" + .to_string() +} + +pub(crate) fn empty_tag_summary() -> PatchTagSummary { + PatchTagSummary { + reviewed_by: HashSet::new(), + tested_by: HashSet::new(), + acked_by: HashSet::new(), + } +} diff --git a/src/app/integration_tests/helpers/mod.rs b/src/app/integration_tests/helpers/mod.rs new file mode 100644 index 0000000..68091d1 --- /dev/null +++ b/src/app/integration_tests/helpers/mod.rs @@ -0,0 +1,4 @@ +pub(crate) mod app_harness; +pub(crate) mod loading; +pub(crate) mod lore; +pub(crate) mod render; diff --git a/src/app/integration_tests/helpers/render.rs b/src/app/integration_tests/helpers/render.rs new file mode 100644 index 0000000..e82741c --- /dev/null +++ b/src/app/integration_tests/helpers/render.rs @@ -0,0 +1,7 @@ +use crate::render::{RenderedPatchPreview, RenderedPatchsetPreview}; + +pub(crate) fn sample_rendered_preview() -> RenderedPatchsetPreview { + RenderedPatchsetPreview::new(vec![RenderedPatchPreview::new( + "Subject: [PATCH] test\n---\n+added line".to_string(), + )]) +} diff --git a/src/app/integration_tests/mod.rs b/src/app/integration_tests/mod.rs index 44c708e..bacaf26 100644 --- a/src/app/integration_tests/mod.rs +++ b/src/app/integration_tests/mod.rs @@ -1,6 +1,43 @@ +mod helpers; + +use crate::app::{loading::LoadingIndicator, screens::CurrentScreen}; + +use helpers::{ + app_harness::AppHarness, + loading::FakeLoadingIndicator, + lore::{sample_patch, sample_patchset_details}, + render::sample_rendered_preview, +}; + #[test] -fn integration_test_module_is_wired() { - // Keeps the module compiling before the shared harness and behavior tests - // are introduced in follow-up slices. - assert!(true); +fn helper_builds_minimal_app_on_initial_screen() { + let harness = AppHarness::new(); + + assert_eq!( + CurrentScreen::MailingListSelection, + harness.app.state.navigation.current_screen + ); +} + +#[test] +fn helpers_provide_sample_patchset_inputs() { + let patch = sample_patch(); + let details = sample_patchset_details(); + let rendered = sample_rendered_preview(); + + assert_eq!("[PATCH 1/1] test patch", patch.title()); + assert_eq!(1, details.raw_patches.len()); + assert_eq!(1, details.tag_summary.len()); + assert_eq!(1, rendered.entries.len()); +} + +#[test] +fn fake_loading_indicator_records_start_and_stop() { + let mut loading = FakeLoadingIndicator::default(); + + loading.start("Loading patchset".to_string()); + loading.stop().unwrap(); + + assert_eq!(vec!["Loading patchset"], loading.starts); + assert_eq!(1, loading.stop_count); } From d81eb3250e30682c8b62336d562e23bb5aead50f Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 9 Jul 2026 16:14:03 -0300 Subject: [PATCH 14/18] test(app): add navigation flow integration tests This commit covers latest and bookmarked patchset navigation through the app flow handlers. The tests exercise opening patchset details from both origins and returning with Back using helper-backed Lore and Render handles. This commit is part of the architecture's refactoring phase 15. Signed-off-by: lorenzoberts --- src/app/integration_tests/flow_navigation.rs | 117 ++++++++++++++++++ .../integration_tests/helpers/app_harness.rs | 36 +++++- src/app/integration_tests/helpers/lore.rs | 26 +++- src/app/integration_tests/helpers/render.rs | 21 +++- src/app/integration_tests/mod.rs | 1 + 5 files changed, 195 insertions(+), 6 deletions(-) create mode 100644 src/app/integration_tests/flow_navigation.rs diff --git a/src/app/integration_tests/flow_navigation.rs b/src/app/integration_tests/flow_navigation.rs new file mode 100644 index 0000000..6553525 --- /dev/null +++ b/src/app/integration_tests/flow_navigation.rs @@ -0,0 +1,117 @@ +use std::ops::ControlFlow; + +use crate::{ + app::{ + flows::{ + bookmarked::handle_bookmarked_patchsets, details_actions::handle_patchset_details, + latest::handle_latest_patchsets, mail_list::handle_mailing_list_selection, + }, + screens::CurrentScreen, + }, + input::event::InputEvent, +}; + +use super::helpers::{ + app_harness::{dummy_terminal_handle, AppHarness}, + loading::FakeLoadingIndicator, + lore::lore_handle_with_successful_patch_flow, + render::render_handle_with_successful_preview, +}; + +#[tokio::test] +async fn latest_list_opens_details_and_back_returns_to_latest() { + let mut harness = AppHarness::with_handles( + lore_handle_with_successful_patch_flow(), + render_handle_with_successful_preview(), + ); + let mut loading = FakeLoadingIndicator::default(); + + let result = handle_mailing_list_selection( + &mut harness.app, + InputEvent::OpenLatestPatchsets, + &mut loading, + ) + .await + .unwrap(); + assert_eq!(ControlFlow::Continue(()), result); + assert_eq!( + CurrentScreen::LatestPatchsets, + harness.app.state.navigation.current_screen + ); + + handle_latest_patchsets( + &mut harness.app, + InputEvent::OpenPatchsetDetails, + &mut loading, + ) + .await + .unwrap(); + assert_eq!( + CurrentScreen::PatchsetDetails, + harness.app.state.navigation.current_screen + ); + assert!(harness.app.state.lore.details.is_some()); + + handle_patchset_details( + &mut harness.app, + InputEvent::Back, + &dummy_terminal_handle(), + ) + .await + .unwrap(); + + assert_eq!( + CurrentScreen::LatestPatchsets, + harness.app.state.navigation.current_screen + ); + assert!(harness.app.state.lore.details.is_none()); +} + +#[tokio::test] +async fn bookmarked_list_opens_details_and_back_returns_to_bookmarks() { + let mut harness = AppHarness::with_bookmark( + lore_handle_with_successful_patch_flow(), + render_handle_with_successful_preview(), + ); + let mut loading = FakeLoadingIndicator::default(); + + let result = handle_mailing_list_selection( + &mut harness.app, + InputEvent::OpenBookmarkedPatchsets, + &mut loading, + ) + .await + .unwrap(); + assert_eq!(ControlFlow::Continue(()), result); + assert_eq!( + CurrentScreen::BookmarkedPatchsets, + harness.app.state.navigation.current_screen + ); + + handle_bookmarked_patchsets( + &mut harness.app, + InputEvent::OpenPatchsetDetails, + &mut loading, + ) + .await + .unwrap(); + assert_eq!( + CurrentScreen::PatchsetDetails, + harness.app.state.navigation.current_screen + ); + assert!(harness.app.state.lore.details.is_some()); + + handle_patchset_details( + &mut harness.app, + InputEvent::Back, + &dummy_terminal_handle(), + ) + .await + .unwrap(); + + assert_eq!( + CurrentScreen::BookmarkedPatchsets, + harness.app.state.navigation.current_screen + ); + assert!(harness.app.state.lore.details.is_none()); +} diff --git a/src/app/integration_tests/helpers/app_harness.rs b/src/app/integration_tests/helpers/app_harness.rs index 5141023..5ce5fd6 100644 --- a/src/app/integration_tests/helpers/app_harness.rs +++ b/src/app/integration_tests/helpers/app_harness.rs @@ -6,9 +6,10 @@ use crate::{ infrastructure::{file_system::MockFileSystemTrait, shell::MockShellTrait}, lore::application::{cache::BootstrapLoreData, handle::LoreApiHandle}, render::handle::RenderHandle, + terminal::{handle::TerminalHandle, messages::TerminalMessage}, }; -use super::lore::sample_mailing_list; +use super::lore::{sample_mailing_list, sample_patch}; pub(crate) struct AppHarness { pub(crate) app: App, @@ -20,17 +21,39 @@ impl AppHarness { app: minimal_app(), } } + + pub(crate) fn with_handles(lore_api: LoreApiHandle, render: RenderHandle) -> Self { + Self { + app: app_with_bootstrap_and_handles(bootstrap_data(), lore_api, render), + } + } + + pub(crate) fn with_bookmark(lore_api: LoreApiHandle, render: RenderHandle) -> Self { + let mut bootstrap = bootstrap_data(); + bootstrap.bookmarks = vec![sample_patch()]; + Self { + app: app_with_bootstrap_and_handles(bootstrap, lore_api, render), + } + } } pub(crate) fn minimal_app() -> App { + app_with_bootstrap_and_handles(bootstrap_data(), dummy_lore_handle(), dummy_render_handle()) +} + +pub(crate) fn app_with_bootstrap_and_handles( + bootstrap: BootstrapLoreData, + lore_api: LoreApiHandle, + render: RenderHandle, +) -> App { App::new( ConfigState::default().to_snapshot(), dummy_config_handle(), - bootstrap_data(), + bootstrap, Box::new(MockFileSystemTrait::new()), Box::new(MockShellTrait::new()), - dummy_lore_handle(), - dummy_render_handle(), + lore_api, + render, ) .expect("minimal app should build") } @@ -50,6 +73,11 @@ pub(crate) fn dummy_render_handle() -> RenderHandle { RenderHandle::new(tx) } +pub(crate) fn dummy_terminal_handle() -> TerminalHandle { + let (tx, _rx) = mpsc::channel::(1); + TerminalHandle::new(tx) +} + fn bootstrap_data() -> BootstrapLoreData { BootstrapLoreData { mailing_lists: vec![sample_mailing_list()], diff --git a/src/app/integration_tests/helpers/lore.rs b/src/app/integration_tests/helpers/lore.rs index 4c8023e..6abd5b9 100644 --- a/src/app/integration_tests/helpers/lore.rs +++ b/src/app/integration_tests/helpers/lore.rs @@ -1,12 +1,36 @@ use std::collections::HashSet; use serde_xml_rs::from_str; +use tokio::{spawn, sync::mpsc}; use crate::lore::{ - application::dto::{PatchTagSummary, PatchsetDetails}, + application::{ + dto::{PatchTagSummary, PatchsetDetails}, + handle::LoreApiHandle, + messages::LoreApiMessage, + }, domain::{mailing_list::MailingList, patch::Patch}, }; +pub(crate) fn lore_handle_with_successful_patch_flow() -> LoreApiHandle { + let (tx, mut rx) = mpsc::channel(8); + spawn(async move { + while let Some(message) = rx.recv().await { + match message { + LoreApiMessage::FetchFeedPage { reply, .. } => { + reply.send(Ok(vec![sample_patch()])).ok(); + } + LoreApiMessage::FetchPatchsetDetails { reply, .. } => { + reply.send(Ok(sample_patchset_details())).ok(); + } + LoreApiMessage::Shutdown => break, + other => panic!("unexpected lore message: {}", other.name()), + } + } + }); + LoreApiHandle::new(tx) +} + pub(crate) fn sample_mailing_list() -> MailingList { MailingList::new("test-list", "Test list") } diff --git a/src/app/integration_tests/helpers/render.rs b/src/app/integration_tests/helpers/render.rs index e82741c..41f5221 100644 --- a/src/app/integration_tests/helpers/render.rs +++ b/src/app/integration_tests/helpers/render.rs @@ -1,4 +1,23 @@ -use crate::render::{RenderedPatchPreview, RenderedPatchsetPreview}; +use tokio::{spawn, sync::mpsc}; + +use crate::render::{ + handle::RenderHandle, messages::RenderMessage, RenderedPatchPreview, RenderedPatchsetPreview, +}; + +pub(crate) fn render_handle_with_successful_preview() -> RenderHandle { + let (tx, mut rx) = mpsc::channel(8); + spawn(async move { + while let Some(message) = rx.recv().await { + match message { + RenderMessage::RenderPatchsetPreview { reply, .. } => { + reply.send(Ok(sample_rendered_preview())).ok(); + } + RenderMessage::Shutdown => break, + } + } + }); + RenderHandle::new(tx) +} pub(crate) fn sample_rendered_preview() -> RenderedPatchsetPreview { RenderedPatchsetPreview::new(vec![RenderedPatchPreview::new( diff --git a/src/app/integration_tests/mod.rs b/src/app/integration_tests/mod.rs index bacaf26..0d69ece 100644 --- a/src/app/integration_tests/mod.rs +++ b/src/app/integration_tests/mod.rs @@ -1,3 +1,4 @@ +mod flow_navigation; mod helpers; use crate::app::{loading::LoadingIndicator, screens::CurrentScreen}; From 59facd00e192c451bb02ba7ea846140af55fe342 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 9 Jul 2026 16:15:51 -0300 Subject: [PATCH 15/18] test(app): add patch-open error flow integration tests This commit covers app behavior when opening patchset details fails from latest and bookmarked origins. The tests assert that render and Lore failures surface as error popups, preserve the originating screen, and avoid leaving stale details state behind. This commit is part of the architecture's refactoring phase 15. Signed-off-by: lorenzoberts --- src/app/integration_tests/flow_errors.rs | 114 ++++++++++++++++++++ src/app/integration_tests/helpers/lore.rs | 21 ++++ src/app/integration_tests/helpers/render.rs | 22 +++- src/app/integration_tests/mod.rs | 1 + 4 files changed, 157 insertions(+), 1 deletion(-) create mode 100644 src/app/integration_tests/flow_errors.rs diff --git a/src/app/integration_tests/flow_errors.rs b/src/app/integration_tests/flow_errors.rs new file mode 100644 index 0000000..a41ac61 --- /dev/null +++ b/src/app/integration_tests/flow_errors.rs @@ -0,0 +1,114 @@ +use std::ops::ControlFlow; + +use crate::{ + app::{ + flows::{ + bookmarked::handle_bookmarked_patchsets, latest::handle_latest_patchsets, + mail_list::handle_mailing_list_selection, + }, + popup::AppPopup, + screens::CurrentScreen, + }, + input::event::InputEvent, +}; + +use super::helpers::{ + app_harness::AppHarness, + loading::FakeLoadingIndicator, + lore::{lore_handle_with_patch_details_failure, lore_handle_with_successful_patch_flow}, + render::{render_handle_with_preview_failure, render_handle_with_successful_preview}, +}; + +#[tokio::test] +async fn latest_render_failure_shows_popup_and_stays_on_latest() { + let mut harness = AppHarness::with_handles( + lore_handle_with_successful_patch_flow(), + render_handle_with_preview_failure(), + ); + let mut loading = FakeLoadingIndicator::default(); + + let result = handle_mailing_list_selection( + &mut harness.app, + InputEvent::OpenLatestPatchsets, + &mut loading, + ) + .await + .unwrap(); + assert_eq!(ControlFlow::Continue(()), result); + + handle_latest_patchsets( + &mut harness.app, + InputEvent::OpenPatchsetDetails, + &mut loading, + ) + .await + .unwrap(); + + assert_eq!( + CurrentScreen::LatestPatchsets, + harness.app.state.navigation.current_screen + ); + assert!(harness.app.state.lore.details.is_none()); + assert_info_popup_contains( + harness.app.state.popup.as_ref(), + "Error", + &[ + "The selected patchset couldn't be opened.", + "render actor unavailable in test", + ], + ); +} + +#[tokio::test] +async fn bookmarked_lore_failure_shows_popup_and_stays_on_bookmarks() { + let mut harness = AppHarness::with_bookmark( + lore_handle_with_patch_details_failure(), + render_handle_with_successful_preview(), + ); + let mut loading = FakeLoadingIndicator::default(); + + let result = handle_mailing_list_selection( + &mut harness.app, + InputEvent::OpenBookmarkedPatchsets, + &mut loading, + ) + .await + .unwrap(); + assert_eq!(ControlFlow::Continue(()), result); + + handle_bookmarked_patchsets( + &mut harness.app, + InputEvent::OpenPatchsetDetails, + &mut loading, + ) + .await + .unwrap(); + + assert_eq!( + CurrentScreen::BookmarkedPatchsets, + harness.app.state.navigation.current_screen + ); + assert!(harness.app.state.lore.details.is_none()); + assert_info_popup_contains( + harness.app.state.popup.as_ref(), + "Error", + &[ + "The selected patchset couldn't be opened.", + "lore actor unavailable in test", + ], + ); +} + +fn assert_info_popup_contains(popup: Option<&AppPopup>, expected_title: &str, expected: &[&str]) { + let Some(AppPopup::Info { title, body, .. }) = popup else { + panic!("expected info popup"); + }; + + assert_eq!(expected_title, title); + for fragment in expected { + assert!( + body.contains(fragment), + "expected popup body to contain {fragment:?}, got {body:?}" + ); + } +} diff --git a/src/app/integration_tests/helpers/lore.rs b/src/app/integration_tests/helpers/lore.rs index 6abd5b9..04f7e3b 100644 --- a/src/app/integration_tests/helpers/lore.rs +++ b/src/app/integration_tests/helpers/lore.rs @@ -6,6 +6,7 @@ use tokio::{spawn, sync::mpsc}; use crate::lore::{ application::{ dto::{PatchTagSummary, PatchsetDetails}, + errors::LoreError, handle::LoreApiHandle, messages::LoreApiMessage, }, @@ -31,6 +32,26 @@ pub(crate) fn lore_handle_with_successful_patch_flow() -> LoreApiHandle { LoreApiHandle::new(tx) } +pub(crate) fn lore_handle_with_patch_details_failure() -> LoreApiHandle { + let (tx, mut rx) = mpsc::channel(8); + spawn(async move { + while let Some(message) = rx.recv().await { + match message { + LoreApiMessage::FetchPatchsetDetails { reply, .. } => { + reply + .send(Err(LoreError::ActorUnavailable( + "lore actor unavailable in test".to_string(), + ))) + .ok(); + } + LoreApiMessage::Shutdown => break, + other => panic!("unexpected lore message: {}", other.name()), + } + } + }); + LoreApiHandle::new(tx) +} + pub(crate) fn sample_mailing_list() -> MailingList { MailingList::new("test-list", "Test list") } diff --git a/src/app/integration_tests/helpers/render.rs b/src/app/integration_tests/helpers/render.rs index 41f5221..fa04aeb 100644 --- a/src/app/integration_tests/helpers/render.rs +++ b/src/app/integration_tests/helpers/render.rs @@ -1,7 +1,8 @@ use tokio::{spawn, sync::mpsc}; use crate::render::{ - handle::RenderHandle, messages::RenderMessage, RenderedPatchPreview, RenderedPatchsetPreview, + handle::RenderHandle, messages::RenderMessage, RenderError, RenderedPatchPreview, + RenderedPatchsetPreview, }; pub(crate) fn render_handle_with_successful_preview() -> RenderHandle { @@ -19,6 +20,25 @@ pub(crate) fn render_handle_with_successful_preview() -> RenderHandle { RenderHandle::new(tx) } +pub(crate) fn render_handle_with_preview_failure() -> RenderHandle { + let (tx, mut rx) = mpsc::channel(8); + spawn(async move { + while let Some(message) = rx.recv().await { + match message { + RenderMessage::RenderPatchsetPreview { reply, .. } => { + reply + .send(Err(RenderError::ActorUnavailable( + "render actor unavailable in test".to_string(), + ))) + .ok(); + } + RenderMessage::Shutdown => break, + } + } + }); + RenderHandle::new(tx) +} + pub(crate) fn sample_rendered_preview() -> RenderedPatchsetPreview { RenderedPatchsetPreview::new(vec![RenderedPatchPreview::new( "Subject: [PATCH] test\n---\n+added line".to_string(), diff --git a/src/app/integration_tests/mod.rs b/src/app/integration_tests/mod.rs index 0d69ece..ad86b76 100644 --- a/src/app/integration_tests/mod.rs +++ b/src/app/integration_tests/mod.rs @@ -1,3 +1,4 @@ +mod flow_errors; mod flow_navigation; mod helpers; From 73617d2c0ddc05d1a10854d2ae224d98cfcb5883 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 9 Jul 2026 16:17:53 -0300 Subject: [PATCH 16/18] test(app): add apply action integration tests This commit covers app-level apply patchset behavior through consolidate_patchset_actions. The tests assert success and failure popups, verify apply action reset after consolidation, and ensure failed git-am runs abort before returning to the previous branch. This commit is part of the architecture's refactoring phase 15. Signed-off-by: lorenzoberts --- src/app/integration_tests/flow_navigation.rs | 20 +- .../integration_tests/helpers/app_harness.rs | 4 +- src/app/integration_tests/helpers/lore.rs | 19 ++ src/app/integration_tests/mod.rs | 1 + src/app/integration_tests/patchset_actions.rs | 206 ++++++++++++++++++ 5 files changed, 233 insertions(+), 17 deletions(-) create mode 100644 src/app/integration_tests/patchset_actions.rs diff --git a/src/app/integration_tests/flow_navigation.rs b/src/app/integration_tests/flow_navigation.rs index 6553525..fb09436 100644 --- a/src/app/integration_tests/flow_navigation.rs +++ b/src/app/integration_tests/flow_navigation.rs @@ -52,13 +52,9 @@ async fn latest_list_opens_details_and_back_returns_to_latest() { ); assert!(harness.app.state.lore.details.is_some()); - handle_patchset_details( - &mut harness.app, - InputEvent::Back, - &dummy_terminal_handle(), - ) - .await - .unwrap(); + handle_patchset_details(&mut harness.app, InputEvent::Back, &dummy_terminal_handle()) + .await + .unwrap(); assert_eq!( CurrentScreen::LatestPatchsets, @@ -101,13 +97,9 @@ async fn bookmarked_list_opens_details_and_back_returns_to_bookmarks() { ); assert!(harness.app.state.lore.details.is_some()); - handle_patchset_details( - &mut harness.app, - InputEvent::Back, - &dummy_terminal_handle(), - ) - .await - .unwrap(); + handle_patchset_details(&mut harness.app, InputEvent::Back, &dummy_terminal_handle()) + .await + .unwrap(); assert_eq!( CurrentScreen::BookmarkedPatchsets, diff --git a/src/app/integration_tests/helpers/app_harness.rs b/src/app/integration_tests/helpers/app_harness.rs index 5ce5fd6..24f1dff 100644 --- a/src/app/integration_tests/helpers/app_harness.rs +++ b/src/app/integration_tests/helpers/app_harness.rs @@ -17,9 +17,7 @@ pub(crate) struct AppHarness { impl AppHarness { pub(crate) fn new() -> Self { - Self { - app: minimal_app(), - } + Self { app: minimal_app() } } pub(crate) fn with_handles(lore_api: LoreApiHandle, render: RenderHandle) -> Self { diff --git a/src/app/integration_tests/helpers/lore.rs b/src/app/integration_tests/helpers/lore.rs index 04f7e3b..afa791e 100644 --- a/src/app/integration_tests/helpers/lore.rs +++ b/src/app/integration_tests/helpers/lore.rs @@ -52,6 +52,25 @@ pub(crate) fn lore_handle_with_patch_details_failure() -> LoreApiHandle { LoreApiHandle::new(tx) } +pub(crate) fn lore_handle_with_persistence() -> LoreApiHandle { + let (tx, mut rx) = mpsc::channel(8); + spawn(async move { + while let Some(message) = rx.recv().await { + match message { + LoreApiMessage::SaveBookmarks { reply, .. } => { + reply.send(Ok(())).ok(); + } + LoreApiMessage::SaveReviewed { reply, .. } => { + reply.send(Ok(())).ok(); + } + LoreApiMessage::Shutdown => break, + other => panic!("unexpected lore message: {}", other.name()), + } + } + }); + LoreApiHandle::new(tx) +} + pub(crate) fn sample_mailing_list() -> MailingList { MailingList::new("test-list", "Test list") } diff --git a/src/app/integration_tests/mod.rs b/src/app/integration_tests/mod.rs index ad86b76..d5a3b39 100644 --- a/src/app/integration_tests/mod.rs +++ b/src/app/integration_tests/mod.rs @@ -1,6 +1,7 @@ mod flow_errors; mod flow_navigation; mod helpers; +mod patchset_actions; use crate::app::{loading::LoadingIndicator, screens::CurrentScreen}; diff --git a/src/app/integration_tests/patchset_actions.rs b/src/app/integration_tests/patchset_actions.rs new file mode 100644 index 0000000..8176198 --- /dev/null +++ b/src/app/integration_tests/patchset_actions.rs @@ -0,0 +1,206 @@ +use std::{ + collections::VecDeque, + sync::{Arc, Mutex}, +}; + +use crate::{ + app::{ + popup::AppPopup, + screens::{ + details_actions::{PatchsetAction, PatchsetDetailsState}, + CurrentScreen, + }, + App, + }, + config::{ConfigSnapshot, ConfigState}, + infrastructure::{ + file_system::MockFileSystemTrait, + shell::{MockShellTrait, ShellCommand, ShellOutput}, + }, +}; + +use super::helpers::{ + app_harness::{dummy_config_handle, dummy_render_handle}, + lore::{ + lore_handle_with_persistence, sample_mailing_list, sample_patch, sample_patchset_details, + }, + render::sample_rendered_preview, +}; + +const KERNEL_TREE_PATH: &str = "/kernel"; +const BASE_BRANCH: &str = "main"; + +#[tokio::test] +async fn apply_success_sets_success_popup_and_resets_apply_action() { + let (shell, _calls) = shell_with_outputs(vec![ + output("", "", true), + output("", "", true), + output("feature\n", "", true), + output("", "", true), + output("", "", true), + output("", "", true), + output("", "", true), + ]); + let mut app = app_with_apply_details(clean_fs(), shell); + + app.consolidate_patchset_actions().await.unwrap(); + + assert_apply_action(&app, false); + assert_info_popup_contains( + app.state.popup.as_ref(), + "Patchset Apply Success", + &[ + "applied successfully", + "Kernel Tree: '/kernel'", + "Applied branch: 'patchset-", + ], + ); +} + +#[tokio::test] +async fn apply_failure_sets_failure_popup_and_resets_apply_action() { + let (shell, calls) = shell_with_outputs(vec![ + output("", "", true), + output("", "", true), + output("feature\n", "", true), + output("", "", true), + output("", "", true), + output("", "apply failed", false), + output("", "", true), + output("", "", true), + ]); + let mut app = app_with_apply_details(clean_fs(), shell); + + app.consolidate_patchset_actions().await.unwrap(); + + assert_apply_action(&app, false); + assert_info_popup_contains( + app.state.popup.as_ref(), + "Patchset Apply Fail", + &["`git am` failed", "feature", "apply failed"], + ); + let calls = calls.lock().unwrap(); + assert_eq!( + command(&["git", "-C", KERNEL_TREE_PATH, "am", "--abort"]), + calls[6] + ); +} + +fn app_with_apply_details(fs: MockFileSystemTrait, shell: MockShellTrait) -> App { + let mut app = App::new( + apply_config(), + dummy_config_handle(), + crate::lore::application::cache::BootstrapLoreData { + mailing_lists: vec![sample_mailing_list()], + bookmarks: vec![], + reviewed: Default::default(), + }, + Box::new(fs), + Box::new(shell), + lore_handle_with_persistence(), + dummy_render_handle(), + ) + .expect("app should build"); + + app.state.navigation.current_screen = CurrentScreen::PatchsetDetails; + app.state.lore.details = Some(apply_details_state()); + app +} + +fn apply_details_state() -> PatchsetDetailsState { + let mut details = PatchsetDetailsState::from_rendered_preview( + sample_patch(), + sample_patchset_details(), + sample_rendered_preview(), + false, + CurrentScreen::LatestPatchsets, + ); + details.toggle_apply_action(); + details +} + +fn apply_config() -> ConfigSnapshot { + serde_json::from_value::(serde_json::json!({ + "kernel_trees": { + "linux": { + "path": KERNEL_TREE_PATH, + "branch": BASE_BRANCH + } + }, + "target_kernel_tree": "linux", + "git_am_options": "--signoff --3way", + "git_am_branch_prefix": "patchset-" + })) + .expect("test config should deserialize") + .to_snapshot() +} + +fn clean_fs() -> MockFileSystemTrait { + let mut fs = MockFileSystemTrait::new(); + fs.expect_is_dir() + .returning(|path| matches!(path.to_str(), Some("/kernel") | Some("/kernel/.git"))); + fs.expect_is_file().returning(|_| false); + fs +} + +fn shell_with_outputs(outputs: Vec) -> (MockShellTrait, Arc>>>) { + let calls = Arc::new(Mutex::new(Vec::new())); + let outputs = Arc::new(Mutex::new(VecDeque::from(outputs))); + let mut shell = MockShellTrait::new(); + let calls_for_execute = Arc::clone(&calls); + let outputs_for_execute = Arc::clone(&outputs); + shell.expect_execute().returning(move |cmd| { + calls_for_execute.lock().unwrap().push(command_parts(cmd)); + Ok(outputs_for_execute + .lock() + .unwrap() + .pop_front() + .expect("test should provide one output per shell command")) + }); + (shell, calls) +} + +fn output(stdout: impl Into>, stderr: impl Into>, success: bool) -> ShellOutput { + ShellOutput { + stdout: stdout.into(), + stderr: stderr.into(), + success, + } +} + +fn command_parts(cmd: &ShellCommand) -> Vec { + let mut parts = vec![cmd.program.clone()]; + parts.extend(cmd.args.clone()); + parts +} + +fn command(parts: &[&str]) -> Vec { + parts.iter().map(|part| part.to_string()).collect() +} + +fn assert_apply_action(app: &App, expected: bool) { + let details = app + .state + .lore + .details + .as_ref() + .expect("details should remain loaded"); + assert_eq!( + Some(&expected), + details.patchset_actions.get(&PatchsetAction::Apply) + ); +} + +fn assert_info_popup_contains(popup: Option<&AppPopup>, expected_title: &str, expected: &[&str]) { + let Some(AppPopup::Info { title, body, .. }) = popup else { + panic!("expected info popup"); + }; + + assert_eq!(expected_title, title); + for fragment in expected { + assert!( + body.contains(fragment), + "expected popup body to contain {fragment:?}, got {body:?}" + ); + } +} From 3821dd031f235e7d15c1683ad5ff96768cf6980a Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 9 Jul 2026 16:19:38 -0300 Subject: [PATCH 17/18] test(app): add reviewed-by action integration tests This commit covers app-level reviewed-by reply behavior through consolidate_patchset_actions. The tests assert successful replies record and persist reviewed indexes, failed interactive replies do not, and reply selection state is reset after consolidation. This commit is part of the architecture's refactoring phase 15. Signed-off-by: lorenzoberts --- src/app/integration_tests/patchset_actions.rs | 174 +++++++++++++++++- 1 file changed, 170 insertions(+), 4 deletions(-) diff --git a/src/app/integration_tests/patchset_actions.rs b/src/app/integration_tests/patchset_actions.rs index 8176198..9aae9e7 100644 --- a/src/app/integration_tests/patchset_actions.rs +++ b/src/app/integration_tests/patchset_actions.rs @@ -1,8 +1,10 @@ use std::{ - collections::VecDeque, + collections::{HashMap, HashSet, VecDeque}, sync::{Arc, Mutex}, }; +use tokio::{spawn, sync::mpsc}; + use crate::{ app::{ popup::AppPopup, @@ -17,6 +19,9 @@ use crate::{ file_system::MockFileSystemTrait, shell::{MockShellTrait, ShellCommand, ShellOutput}, }, + lore::application::{ + cache::BootstrapLoreData, handle::LoreApiHandle, messages::LoreApiMessage, + }, }; use super::helpers::{ @@ -86,24 +91,108 @@ async fn apply_failure_sets_failure_popup_and_resets_apply_action() { ); } +#[tokio::test] +async fn reviewed_reply_success_records_persists_and_resets_reply_action() { + let saved_reviewed = Arc::new(Mutex::new(None)); + let lore_api = reviewed_reply_lore_handle(Arc::clone(&saved_reviewed)); + let mut shell = MockShellTrait::new(); + shell + .expect_execute() + .withf(|cmd| cmd.program == "mktemp" && cmd.args == ["--directory"]) + .times(1) + .returning(|_| Ok(output("/tmp/reviewed-reply\n", "", true))); + shell + .expect_spawn_interactive() + .times(1) + .returning(|_| Ok(true)); + let mut app = app_with_reviewed_reply_details(shell, lore_api); + let message_id = selected_message_id(&app); + + app.consolidate_patchset_actions().await.unwrap(); + + assert_reply_action_reset(&app); + assert_eq!( + HashSet::from([0]), + app.state.user_state.reviewed_patchsets[&message_id] + ); + let saved = saved_reviewed + .lock() + .unwrap() + .clone() + .expect("reviewed state should be persisted"); + assert_eq!(HashSet::from([0]), saved[&message_id]); +} + +#[tokio::test] +async fn reviewed_reply_failure_does_not_record_failed_index() { + let saved_reviewed = Arc::new(Mutex::new(None)); + let lore_api = reviewed_reply_lore_handle(Arc::clone(&saved_reviewed)); + let mut shell = MockShellTrait::new(); + shell + .expect_execute() + .withf(|cmd| cmd.program == "mktemp" && cmd.args == ["--directory"]) + .times(1) + .returning(|_| Ok(output("/tmp/reviewed-reply\n", "", true))); + shell + .expect_spawn_interactive() + .times(1) + .returning(|_| Ok(false)); + let mut app = app_with_reviewed_reply_details(shell, lore_api); + let message_id = selected_message_id(&app); + + app.consolidate_patchset_actions().await.unwrap(); + + assert_reply_action_reset(&app); + assert!(app.state.user_state.reviewed_patchsets[&message_id].is_empty()); + let saved = saved_reviewed + .lock() + .unwrap() + .clone() + .expect("reviewed state should be persisted"); + assert!(saved[&message_id].is_empty()); +} + fn app_with_apply_details(fs: MockFileSystemTrait, shell: MockShellTrait) -> App { + app_with_details( + fs, + shell, + lore_handle_with_persistence(), + apply_details_state(), + ) +} + +fn app_with_reviewed_reply_details(shell: MockShellTrait, lore_api: LoreApiHandle) -> App { + app_with_details( + MockFileSystemTrait::new(), + shell, + lore_api, + reviewed_reply_details_state(), + ) +} + +fn app_with_details( + fs: MockFileSystemTrait, + shell: MockShellTrait, + lore_api: LoreApiHandle, + details: PatchsetDetailsState, +) -> App { let mut app = App::new( apply_config(), dummy_config_handle(), - crate::lore::application::cache::BootstrapLoreData { + BootstrapLoreData { mailing_lists: vec![sample_mailing_list()], bookmarks: vec![], reviewed: Default::default(), }, Box::new(fs), Box::new(shell), - lore_handle_with_persistence(), + lore_api, dummy_render_handle(), ) .expect("app should build"); app.state.navigation.current_screen = CurrentScreen::PatchsetDetails; - app.state.lore.details = Some(apply_details_state()); + app.state.lore.details = Some(details); app } @@ -119,6 +208,55 @@ fn apply_details_state() -> PatchsetDetailsState { details } +fn reviewed_reply_details_state() -> PatchsetDetailsState { + let mut details = PatchsetDetailsState::from_rendered_preview( + sample_patch(), + sample_patchset_details(), + sample_rendered_preview(), + false, + CurrentScreen::LatestPatchsets, + ); + details.toggle_reply_with_reviewed_by_action(false); + details +} + +fn reviewed_reply_lore_handle( + saved_reviewed: Arc>>>>, +) -> LoreApiHandle { + let (tx, mut rx) = mpsc::channel(8); + spawn(async move { + while let Some(message) = rx.recv().await { + match message { + LoreApiMessage::SaveBookmarks { reply, .. } => { + reply.send(Ok(())).ok(); + } + LoreApiMessage::GetGitSignature { reply, .. } => { + reply + .send(Ok(( + "Reviewer".to_string(), + "reviewer@example.com".to_string(), + ))) + .ok(); + } + LoreApiMessage::PrepareReplyCommands { reply, .. } => { + reply + .send(Ok(vec![ + ShellCommand::new("git").args(["send-email", "--annotate"]) + ])) + .ok(); + } + LoreApiMessage::SaveReviewed { reviewed, reply } => { + *saved_reviewed.lock().unwrap() = Some(reviewed); + reply.send(Ok(())).ok(); + } + LoreApiMessage::Shutdown => break, + other => panic!("unexpected lore message: {}", other.name()), + } + } + }); + LoreApiHandle::new(tx) +} + fn apply_config() -> ConfigSnapshot { serde_json::from_value::(serde_json::json!({ "kernel_trees": { @@ -191,6 +329,34 @@ fn assert_apply_action(app: &App, expected: bool) { ); } +fn selected_message_id(app: &App) -> String { + app.state + .lore + .details + .as_ref() + .expect("details should remain loaded") + .representative_patch + .message_id() + .href + .clone() +} + +fn assert_reply_action_reset(app: &App) { + let details = app + .state + .lore + .details + .as_ref() + .expect("details should remain loaded"); + assert_eq!( + Some(&false), + details + .patchset_actions + .get(&PatchsetAction::ReplyWithReviewedBy) + ); + assert_eq!(vec![false], details.patches_to_reply); +} + fn assert_info_popup_contains(popup: Option<&AppPopup>, expected_title: &str, expected: &[&str]) { let Some(AppPopup::Info { title, body, .. }) = popup else { panic!("expected info popup"); From 73a9ff7d9c0f5ba4fad919eb0636bd136e38ae72 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 9 Jul 2026 16:21:09 -0300 Subject: [PATCH 18/18] test(app): add actor lifecycle integration test This commit covers main-like actor lifecycle wiring with AppActor, InputActor, UiActor, and TerminalActor. The test stops the app through a semantic quit event and verifies input shutdown completes before terminal shutdown, matching the documented teardown order. This commit completes the architecture's refactoring phase 15. Signed-off-by: lorenzoberts --- src/app/actions/reviewed_reply.rs | 10 ++- src/app/integration_tests/actor_lifecycle.rs | 66 +++++++++++++++++++ src/app/integration_tests/mod.rs | 1 + src/app/integration_tests/patchset_actions.rs | 7 +- 4 files changed, 75 insertions(+), 9 deletions(-) create mode 100644 src/app/integration_tests/actor_lifecycle.rs diff --git a/src/app/actions/reviewed_reply.rs b/src/app/actions/reviewed_reply.rs index 51bba6e..25a7ba7 100644 --- a/src/app/actions/reviewed_reply.rs +++ b/src/app/actions/reviewed_reply.rs @@ -169,12 +169,10 @@ mod tests { #[test] fn record_successful_reply_indexes_treats_shell_error_as_failure() { let mut shell = MockShellTrait::new(); - shell.expect_spawn_interactive().times(1).returning(|_| { - Err(ShellError::IoError(io::Error::new( - io::ErrorKind::Other, - "failed", - ))) - }); + shell + .expect_spawn_interactive() + .times(1) + .returning(|_| Err(ShellError::IoError(io::Error::other("failed")))); let mut successful_indexes = HashSet::new(); record_successful_reply_indexes( diff --git a/src/app/integration_tests/actor_lifecycle.rs b/src/app/integration_tests/actor_lifecycle.rs new file mode 100644 index 0000000..acbdc7d --- /dev/null +++ b/src/app/integration_tests/actor_lifecycle.rs @@ -0,0 +1,66 @@ +use std::sync::{ + atomic::{AtomicBool, Ordering}, + Arc, +}; + +use tokio::sync::mpsc; + +use crate::{ + app::actor::AppActor, + input::{actor::InputActor, event::InputEvent}, + terminal::{actor::TerminalActor, messages::TerminalFrame, session::MockTerminalSessionApi}, + ui::actor::UiActor, +}; + +use super::helpers::app_harness::minimal_app; + +#[tokio::test(flavor = "multi_thread")] +async fn main_like_lifecycle_shuts_input_down_before_terminal() { + let input_shutdown_complete = Arc::new(AtomicBool::new(false)); + let input_shutdown_complete_for_terminal = Arc::clone(&input_shutdown_complete); + + let mut session = MockTerminalSessionApi::new(); + session + .expect_draw() + .withf(|frame| matches!(frame, TerminalFrame::Main(_))) + .times(1..) + .returning(|_| Ok(())); + session.expect_poll_event().returning(|_| Ok(None)); + session.expect_shutdown().times(1).returning(move || { + assert!( + input_shutdown_complete_for_terminal.load(Ordering::SeqCst), + "terminal shutdown should happen after input shutdown" + ); + Ok(()) + }); + + let terminal_handle = TerminalActor::spawn(Box::new(session)); + let ui_handle = UiActor::spawn(); + let app = minimal_app(); + let initial_input_context = app.input_context(); + + let input_handle = InputActor::spawn(terminal_handle.clone(), initial_input_context); + let input_shutdown_handle = input_handle.clone(); + let (app_input_tx, app_input_rx) = mpsc::channel::(8); + input_handle + .subscribe_app(app_input_tx.clone()) + .await + .unwrap(); + + let app_handle = AppActor::spawn( + app, + terminal_handle.clone(), + ui_handle.clone(), + input_handle, + app_input_rx, + ); + + app_input_tx.send(InputEvent::Quit).await.unwrap(); + app_handle.run_until_done().await.unwrap(); + + input_shutdown_handle.shutdown().await.unwrap(); + input_shutdown_complete.store(true, Ordering::SeqCst); + + ui_handle.shutdown().await; + terminal_handle.shutdown().await.unwrap(); +} diff --git a/src/app/integration_tests/mod.rs b/src/app/integration_tests/mod.rs index d5a3b39..65db299 100644 --- a/src/app/integration_tests/mod.rs +++ b/src/app/integration_tests/mod.rs @@ -1,3 +1,4 @@ +mod actor_lifecycle; mod flow_errors; mod flow_navigation; mod helpers; diff --git a/src/app/integration_tests/patchset_actions.rs b/src/app/integration_tests/patchset_actions.rs index 9aae9e7..31f3eae 100644 --- a/src/app/integration_tests/patchset_actions.rs +++ b/src/app/integration_tests/patchset_actions.rs @@ -35,6 +35,9 @@ use super::helpers::{ const KERNEL_TREE_PATH: &str = "/kernel"; const BASE_BRANCH: &str = "main"; +type ReviewedState = HashMap>; +type SharedReviewedState = Arc>>; + #[tokio::test] async fn apply_success_sets_success_popup_and_resets_apply_action() { let (shell, _calls) = shell_with_outputs(vec![ @@ -220,9 +223,7 @@ fn reviewed_reply_details_state() -> PatchsetDetailsState { details } -fn reviewed_reply_lore_handle( - saved_reviewed: Arc>>>>, -) -> LoreApiHandle { +fn reviewed_reply_lore_handle(saved_reviewed: SharedReviewedState) -> LoreApiHandle { let (tx, mut rx) = mpsc::channel(8); spawn(async move { while let Some(message) = rx.recv().await {