Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 23 additions & 6 deletions src/api/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,13 +82,19 @@ pub fn format_linked_issue(issue: &LinkedIssue) -> String {
}
}

/// Format the Detail-generated fix PR (number, state, and URL) for the
/// detail/show view, rendered as `#<number> (<state>) — <url>`.
/// Format the Detail-generated fix PR for the detail/show view.
///
/// Rendered as `#<number> (<state>) — <url>`. The URL and its separator are
/// omitted when the API returns an empty URL.
pub fn format_fix_pr(fix_pr: &FixPr) -> String {
format!(
"#{} ({}) \u{2014} {}",
fix_pr.pr_number, fix_pr.state, fix_pr.url
)
if fix_pr.url.is_empty() {
format!("#{} ({})", fix_pr.pr_number, fix_pr.state)
} else {
format!(
"#{} ({}) \u{2014} {}",
fix_pr.pr_number, fix_pr.state, fix_pr.url
)
}
}

// ── clap::ValueEnum ──────────────────────────────────────────────────
Expand Down Expand Up @@ -502,6 +508,17 @@ mod tests {
);
}

#[test]
fn format_fix_pr_omits_separator_when_url_empty() {
let fix_pr: FixPr = serde_json::from_value(serde_json::json!({
"prNumber": 42,
"url": "",
"state": "merged"
}))
.expect("valid FixPr JSON");
assert_eq!(format_fix_pr(&fix_pr), "#42 (merged)");
}

// ── format_introduced_in ─────────────────────────────────────────

#[test]
Expand Down
7 changes: 4 additions & 3 deletions src/commands/bugs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use crate::api::types::{
review_state_label, Bug, BugDismissalReason, BugId, BugReviewState,
ListPublicBugsWorkflowRequestId, RepoId,
};
use crate::output::{output_list, SectionRenderer};
use crate::output::{clamp_page, output_list, SectionRenderer};
use crate::utils::datetime::{format_datetime, parse_time_spec};
use crate::utils::pagination::page_to_offset;
use crate::utils::repos::resolve_repo_id;
Expand Down Expand Up @@ -574,8 +574,9 @@ pub async fn handle(command: &BugCommands, cli: &crate::Cli) -> Result<()> {
let effective_limit = u32::try_from(total.max(1)).unwrap_or(u32::MAX);
return output_list(&filtered, total, 1, effective_limit, format);
}
let page_items = paginate_items(&filtered, *page, *limit);
output_list(&page_items, total, *page, *limit, format)
let page = clamp_page(*page, total, *limit);
let page_items = paginate_items(&filtered, page, *limit);
output_list(&page_items, total, page, *limit, format)
} else if multi_status {
// Multiple statuses but no client-side filters: fetch one
// page per status and merge, avoiding a full exhaust.
Expand Down
7 changes: 4 additions & 3 deletions src/commands/scans.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use clap::Subcommand;

use crate::api::client::ApiClient;
use crate::api::types::{RepoId, Scan, ScanType, ScansResponse, WorkflowStatus};
use crate::output::output_list;
use crate::output::{clamp_page, output_list};
use crate::utils::datetime::parse_time_spec;
use crate::utils::pagination::page_to_offset;
use crate::utils::repos::resolve_repo_id;
Expand Down Expand Up @@ -157,8 +157,9 @@ pub async fn handle(command: &ScanCommands, cli: &crate::Cli) -> Result<()> {
until_ms,
);
let total = filtered.len();
let page_items = paginate_items(&filtered, *page, *limit);
output_list(&page_items, total, *page, *limit, format)
let page = clamp_page(*page, total, *limit);
let page_items = paginate_items(&filtered, page, *limit);
output_list(&page_items, total, page, *limit, format)
} else {
let offset = page_to_offset(*page, *limit);
let scans = client
Expand Down
30 changes: 30 additions & 0 deletions src/output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,14 @@ fn total_pages(total: usize, limit: u32) -> u32 {
.max(1)
}

/// Clamp a page number to the last page that exists.
///
/// Client-side filtering can shrink a result set below the requested page,
/// which would otherwise render an impossible `Page: 2 of 1`.
pub fn clamp_page(page: u32, total: usize, limit: u32) -> u32 {
page.clamp(1, total_pages(total, limit))
}

/// Generic helper to output a list of items in the requested format
pub fn output_list<T: Formattable + Serialize>(
items: &[T],
Expand Down Expand Up @@ -197,6 +205,28 @@ mod tests {
assert_eq!(total_pages(10, 0), 1);
}

// ── clamp_page ───────────────────────────────────────────────────

#[test]
fn clamp_page_keeps_valid_page() {
assert_eq!(clamp_page(2, 100, 50), 2);
}

#[test]
fn clamp_page_lowers_page_beyond_last() {
assert_eq!(clamp_page(5, 10, 50), 1);
}

#[test]
fn clamp_page_empty_results_is_first_page() {
assert_eq!(clamp_page(3, 0, 50), 1);
}

#[test]
fn clamp_page_raises_zero_to_first_page() {
assert_eq!(clamp_page(0, 100, 50), 1);
}

// ── SectionRenderer builder ──────────────────────────────────────

#[test]
Expand Down
35 changes: 32 additions & 3 deletions src/utils/vcs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,18 @@ pub fn repo_root() -> Result<PathBuf> {
}

/// Extract the `origin` remote's URL from `jj git remote list` output,
/// which prints one `name url` pair per line.
/// which prints one `name url` pair per line. When a remote's fetch and push
/// URLs differ, jj appends a ` (push: <url>)` annotation that is dropped here
/// so only the fetch URL is returned.
fn parse_jj_remote_list(list: &str) -> Option<String> {
list.lines()
.find_map(|line| line.strip_prefix("origin "))
.map(|url| url.trim().to_string())
.map(|url| {
url.split_once(" (push:")
.map_or(url, |(fetch, _)| fetch)
.trim()
.to_string()
})
}

/// Extract `owner/repo` from a GitHub remote URL.
Expand Down Expand Up @@ -94,7 +101,9 @@ fn strip_http_credentials(url: &str) -> Option<String> {
// (or end of string). Anything after the authority — including a
// `@` in the path — stays untouched.
let (authority, path) = rest.split_once('/').unwrap_or((rest, ""));
if let Some((_, host)) = authority.split_once('@') {
// The credentials/host separator is the *last* `@`, since a
// password or token may itself contain `@`.
if let Some((_, host)) = authority.rsplit_once('@') {
return Some(format!("{scheme}{host}/{path}"));
}
}
Expand Down Expand Up @@ -205,6 +214,17 @@ mod tests {
);
}

#[test]
fn jj_remote_list_strips_push_url_annotation() {
// jj appends `(push: <url>)` when fetch and push URLs differ.
assert_eq!(
parse_jj_remote_list(
"origin https://github.com/usedetail/cli.git (push: https://other.com/cli.git)"
),
Some("https://github.com/usedetail/cli.git".to_string()),
);
}

// ── parse_github_remote_url ─────────────────────────────────────

#[test]
Expand Down Expand Up @@ -330,6 +350,15 @@ mod tests {
);
}

#[test]
fn parses_https_with_at_sign_in_password() {
// The credentials/host separator is the last `@`, not the first.
assert_eq!(
parse_github_remote_url("https://user:p@ss@github.com/usedetail/cli.git"),
Some("usedetail/cli".to_string()),
);
}

#[test]
fn parses_http_with_credentials() {
assert_eq!(
Expand Down
Loading