From c973550e4e13205eace6ce9a530a5ea3c5f3caae Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 23 Apr 2026 20:59:32 -0400 Subject: [PATCH] refactor(pr): simplify post-refactor PR command code - Parallelize `pr list` discovery loop via buffer_unordered; thread RunId through the stream to drop the run_id.parse().expect(...) panic path. - Skip computing the default model in create_run_pull_request when the request already supplies one (common path from `fabro pr create`). - Delete the dead user_config::storage_dir wrapper (test-only, zero callers, stale deprecation note); point its tests at local_server::storage_dir directly. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-cli/src/commands/pr/list.rs | 87 +++++++++++--------- lib/crates/fabro-cli/src/user_config.rs | 29 +++---- lib/crates/fabro-server/src/server.rs | 8 +- 3 files changed, 64 insertions(+), 60 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/pr/list.rs b/lib/crates/fabro-cli/src/commands/pr/list.rs index bdf994756..fd5415b42 100644 --- a/lib/crates/fabro-cli/src/commands/pr/list.rs +++ b/lib/crates/fabro-cli/src/commands/pr/list.rs @@ -1,6 +1,7 @@ use anyhow::Result; use cli_table::format::{Border, Separator}; use cli_table::{Cell, CellStruct, Color, Style, Table}; +use fabro_api::types::PullRequestDetail; use fabro_util::terminal::Styles; use futures::stream::{self, StreamExt}; use serde::Serialize; @@ -8,9 +9,24 @@ use tracing::info; use crate::args::PrListArgs; use crate::command_context::CommandContext; -use crate::server_runs::ServerSummaryLookup; +use crate::server_runs::{ServerRunSummaryInfo, ServerSummaryLookup}; use crate::shared::{color_if, print_json_pretty}; +fn pr_display_state(detail: &PullRequestDetail) -> String { + if detail.merged { + "merged" + } else if detail.draft { + "draft" + } else { + match detail.state.as_str() { + "open" => "open", + "closed" => "closed", + _ => "unknown", + } + } + .to_string() +} + #[derive(Serialize)] struct PrRow { run_id: String, @@ -26,14 +42,24 @@ pub(super) async fn list_command(args: PrListArgs, base_ctx: &CommandContext) -> let printer = ctx.printer(); let lookup = ServerSummaryLookup::from_client(ctx.server().await?).await?; - let mut entries = Vec::new(); - for run in lookup.runs() { - if let Ok(state) = lookup.client().get_run_state(&run.run_id()).await { - if let Some(record) = state.pull_request { - entries.push((run.run_id().to_string(), record)); + let client = lookup.client().clone_for_reuse(); + let run_ids: Vec<_> = lookup + .runs() + .iter() + .map(ServerRunSummaryInfo::run_id) + .collect(); + let entries: Vec<_> = stream::iter(run_ids) + .map(|run_id| { + let client = client.clone_for_reuse(); + async move { + let state = client.get_run_state(&run_id).await.ok()?; + state.pull_request.map(|record| (run_id, record)) } - } - } + }) + .buffer_unordered(10) + .filter_map(|entry| async move { entry }) + .collect() + .await; if entries.is_empty() { if ctx.json_output() { @@ -44,51 +70,34 @@ pub(super) async fn list_command(args: PrListArgs, base_ctx: &CommandContext) -> return Ok(()); } - let client = lookup.client().clone_for_reuse(); let all_rows = stream::iter(entries) .map(|(run_id, record)| { let client = client.clone_for_reuse(); async move { - match client - .get_run_pull_request(&run_id.parse().expect("run id should parse")) - .await - { - Ok(detail) => { - let state = if detail.merged { - "merged".to_string() - } else if detail.draft { - "draft".to_string() - } else if detail.state == "open" { - "open".to_string() - } else if detail.state == "closed" { - "closed".to_string() - } else { - "unknown".to_string() - }; - Ok(PrRow { - run_id, - number: detail.number, - state, - merged: detail.merged, - title: detail.title, - url: detail.html_url, - }) - } + match client.get_run_pull_request(&run_id).await { + Ok(detail) => Ok(PrRow { + run_id: run_id.to_string(), + number: detail.number, + state: pr_display_state(&detail), + merged: detail.merged, + title: detail.title, + url: detail.html_url, + }), Err(err) => { let message = err.to_string(); if message.contains("GitHub integration unavailable on server.") { return Err(err); } - tracing::warn!(run_id, error = %message, "Failed to fetch PR state"); + tracing::warn!(run_id = %run_id, error = %message, "Failed to fetch PR state"); Ok(PrRow { - run_id, + run_id: run_id.to_string(), number: i64::try_from(record.number) .expect("stored pull request number should fit in i64"), - state: "unknown".to_string(), + state: "unknown".to_string(), merged: false, - title: record.title, - url: record.html_url, + title: record.title, + url: record.html_url, }) } } diff --git a/lib/crates/fabro-cli/src/user_config.rs b/lib/crates/fabro-cli/src/user_config.rs index 7b3a4f8a8..d35ba450a 100644 --- a/lib/crates/fabro-cli/src/user_config.rs +++ b/lib/crates/fabro-cli/src/user_config.rs @@ -1,6 +1,4 @@ use std::path::Path; -#[cfg(test)] -use std::path::PathBuf; use std::str::FromStr; use anyhow::Result; @@ -12,8 +10,6 @@ use fabro_util::version::FABRO_VERSION; use tracing::debug; use crate::args::ServerTargetArgs; -#[cfg(test)] -use crate::local_server; pub(crate) fn load_settings() -> anyhow::Result { load_settings_with_config_and_storage_dir(None, None) @@ -55,14 +51,6 @@ pub(crate) fn default_server_target() -> ServerTarget { ServerTarget::unix_socket_path(default_socket_path()).expect("default socket path is absolute") } -#[deprecated( - note = "use local_server::storage_dir for lifecycle; PR commands must move to server-side API" -)] -#[cfg(test)] -pub(crate) fn storage_dir(settings: &SettingsLayer) -> anyhow::Result { - local_server::storage_dir(settings) -} - fn parse_server_target(value: &str) -> Result { ServerTarget::from_str(value) } @@ -96,16 +84,15 @@ pub(crate) fn cli_http_client_builder() -> fabro_http::HttpClientBuilder { } #[cfg(test)] -#[allow( - deprecated, - reason = "the storage_dir tests are exercising the deprecated helper by definition" -)] mod tests { + use std::path::PathBuf; + use fabro_config::parse_settings_layer; use fabro_config::user::default_storage_dir; use super::*; use crate::args::ServerTargetArgs; + use crate::local_server; fn server_target_args(value: Option<&str>) -> ServerTargetArgs { ServerTargetArgs { @@ -225,7 +212,10 @@ url = "https://config.example.com" fn storage_dir_defaults_without_server_auth_methods() { let settings = SettingsLayer::default(); - assert_eq!(storage_dir(&settings).unwrap(), default_storage_dir()); + assert_eq!( + local_server::storage_dir(&settings).unwrap(), + default_storage_dir() + ); } #[test] @@ -239,7 +229,10 @@ root = "/srv/fabro" "#, ); - assert_eq!(storage_dir(&settings).unwrap(), PathBuf::from("/srv/fabro")); + assert_eq!( + local_server::storage_dir(&settings).unwrap(), + PathBuf::from("/srv/fabro") + ); } #[test] diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 8d3ac5415..12a8a0799 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -5317,13 +5317,15 @@ async fn create_run_pull_request( return missing_remote_branch_error(run_branch).into_response(); } - let configured = configured_providers_from_process_env(Some(&state.vault)).await; - let model = body.model.unwrap_or_else(|| { + let model = if let Some(model) = body.model { + model + } else { + let configured = configured_providers_from_process_env(Some(&state.vault)).await; Catalog::builtin() .default_for_configured(&configured) .id .clone() - }); + }; let pull_request = match pull_request::maybe_open_pull_request( &creds,