Skip to content

Commit 2de4743

Browse files
fix: address all four open CLI issues (#323, #326, #328, #329) (#330)
## Summary All four open issues in this repo were small, well-localized correctness bugs, each fixed with a unit test: - **#323** `format_fix_pr` unconditionally appended ` — {url}`, so an empty `FixPr.url` rendered `#42 (merged) — `. Now branches on `url.is_empty()`, matching `format_linked_issue`/`Bug::to_card`. - **#326** `parse_jj_remote_list` returned everything after `origin `, including jj's ` (push: <url>)` annotation, which flowed through as a garbage repo id (`owner/repo.git (push: https:`) that passed validation. Now splits on `" (push:"` and keeps the fetch URL. - **#328** The client-side-filtered `bugs list` / `scans list` paths paginated with the raw requested page, producing `Page: 5 of 1`. New `output::clamp_page(page, total, limit) -> page.clamp(1, total_pages(total, limit))` is applied before `paginate_items`/`output_list` on both filtered paths, so the page number and the items shown stay consistent. - **#329** `strip_http_credentials` used `split_once('@')`; the credentials/host separator is the *last* `@`, so `https://user:p@ss@github.com/owner/repo.git` yielded host `ss@github.com` and inference returned `None`. Switched to `rsplit_once('@')`, matching `strip_ssh_port` in the same file. Note: the local toolchain had to be updated to current stable (`chacha20 0.10.1` requires edition 2024) to build; `cargo fmt --check`, `cargo clippy -- -D warnings`, `cargo test`, and `cargo xtask check` all pass. `cargo clippy --all-targets` on current stable flags two preexisting lints untouched by this PR (`src/commands/completions.rs:59` redundant closure, trailing commas in `tests/integration.rs`); CI only runs `cargo clippy` without `--all-targets`, so they don't gate this PR. Closes #323, closes #326, closes #328, closes #329. Link to Devin session: https://app.devin.ai/sessions/fb903d8badf449c69bcb0e3b20a52a30 Requested by: @sachiniyer <!-- devin-review-badge-begin --> --- <a href="https://app.devin.ai/review/usedetail/cli/pull/330" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open in Devin Review"> </picture> </a> <!-- devin-review-badge-end --> <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Fixes four CLI correctness bugs in PR formatting, remote URL parsing, pagination, and HTTPS credential handling to improve output accuracy and repo inference. Adds unit tests for each fix. - **Bug Fixes** - Omit the em dash and URL when `FixPr.url` is empty in `format_fix_pr` (#323). - Strip jj’s `(push: <url>)` annotation in `parse_jj_remote_list` to keep the fetch URL (#326). - Clamp page numbers after client-side filtering in `bugs list` and `scans list` to keep pages and items consistent (#328). - Split HTTPS credentials on the last `@` so passwords with `@` parse correctly (#329). <sup>Written for commit 936f2b0. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/usedetail/cli/pull/330?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. --> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Sachin Iyer <siyer@detail.dev>
1 parent cc6e84c commit 2de4743

5 files changed

Lines changed: 93 additions & 15 deletions

File tree

‎src/api/types.rs‎

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -82,13 +82,19 @@ pub fn format_linked_issue(issue: &LinkedIssue) -> String {
8282
}
8383
}
8484

85-
/// Format the Detail-generated fix PR (number, state, and URL) for the
86-
/// detail/show view, rendered as `#<number> (<state>) — <url>`.
85+
/// Format the Detail-generated fix PR for the detail/show view.
86+
///
87+
/// Rendered as `#<number> (<state>) — <url>`. The URL and its separator are
88+
/// omitted when the API returns an empty URL.
8789
pub fn format_fix_pr(fix_pr: &FixPr) -> String {
88-
format!(
89-
"#{} ({}) \u{2014} {}",
90-
fix_pr.pr_number, fix_pr.state, fix_pr.url
91-
)
90+
if fix_pr.url.is_empty() {
91+
format!("#{} ({})", fix_pr.pr_number, fix_pr.state)
92+
} else {
93+
format!(
94+
"#{} ({}) \u{2014} {}",
95+
fix_pr.pr_number, fix_pr.state, fix_pr.url
96+
)
97+
}
9298
}
9399

94100
// ── clap::ValueEnum ──────────────────────────────────────────────────
@@ -502,6 +508,17 @@ mod tests {
502508
);
503509
}
504510

511+
#[test]
512+
fn format_fix_pr_omits_separator_when_url_empty() {
513+
let fix_pr: FixPr = serde_json::from_value(serde_json::json!({
514+
"prNumber": 42,
515+
"url": "",
516+
"state": "merged"
517+
}))
518+
.expect("valid FixPr JSON");
519+
assert_eq!(format_fix_pr(&fix_pr), "#42 (merged)");
520+
}
521+
505522
// ── format_introduced_in ─────────────────────────────────────────
506523

507524
#[test]

‎src/commands/bugs.rs‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ use crate::api::types::{
1111
review_state_label, Bug, BugDismissalReason, BugId, BugReviewState,
1212
ListPublicBugsWorkflowRequestId, RepoId,
1313
};
14-
use crate::output::{output_list, SectionRenderer};
14+
use crate::output::{clamp_page, output_list, SectionRenderer};
1515
use crate::utils::datetime::{format_datetime, parse_time_spec};
1616
use crate::utils::pagination::page_to_offset;
1717
use crate::utils::repos::resolve_repo_id;
@@ -574,8 +574,9 @@ pub async fn handle(command: &BugCommands, cli: &crate::Cli) -> Result<()> {
574574
let effective_limit = u32::try_from(total.max(1)).unwrap_or(u32::MAX);
575575
return output_list(&filtered, total, 1, effective_limit, format);
576576
}
577-
let page_items = paginate_items(&filtered, *page, *limit);
578-
output_list(&page_items, total, *page, *limit, format)
577+
let page = clamp_page(*page, total, *limit);
578+
let page_items = paginate_items(&filtered, page, *limit);
579+
output_list(&page_items, total, page, *limit, format)
579580
} else if multi_status {
580581
// Multiple statuses but no client-side filters: fetch one
581582
// page per status and merge, avoiding a full exhaust.

‎src/commands/scans.rs‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ use clap::Subcommand;
33

44
use crate::api::client::ApiClient;
55
use crate::api::types::{RepoId, Scan, ScanType, ScansResponse, WorkflowStatus};
6-
use crate::output::output_list;
6+
use crate::output::{clamp_page, output_list};
77
use crate::utils::datetime::parse_time_spec;
88
use crate::utils::pagination::page_to_offset;
99
use crate::utils::repos::resolve_repo_id;
@@ -157,8 +157,9 @@ pub async fn handle(command: &ScanCommands, cli: &crate::Cli) -> Result<()> {
157157
until_ms,
158158
);
159159
let total = filtered.len();
160-
let page_items = paginate_items(&filtered, *page, *limit);
161-
output_list(&page_items, total, *page, *limit, format)
160+
let page = clamp_page(*page, total, *limit);
161+
let page_items = paginate_items(&filtered, page, *limit);
162+
output_list(&page_items, total, page, *limit, format)
162163
} else {
163164
let offset = page_to_offset(*page, *limit);
164165
let scans = client

‎src/output.rs‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,14 @@ fn total_pages(total: usize, limit: u32) -> u32 {
118118
.max(1)
119119
}
120120

121+
/// Clamp a page number to the last page that exists.
122+
///
123+
/// Client-side filtering can shrink a result set below the requested page,
124+
/// which would otherwise render an impossible `Page: 2 of 1`.
125+
pub fn clamp_page(page: u32, total: usize, limit: u32) -> u32 {
126+
page.clamp(1, total_pages(total, limit))
127+
}
128+
121129
/// Generic helper to output a list of items in the requested format
122130
pub fn output_list<T: Formattable + Serialize>(
123131
items: &[T],
@@ -197,6 +205,28 @@ mod tests {
197205
assert_eq!(total_pages(10, 0), 1);
198206
}
199207

208+
// ── clamp_page ───────────────────────────────────────────────────
209+
210+
#[test]
211+
fn clamp_page_keeps_valid_page() {
212+
assert_eq!(clamp_page(2, 100, 50), 2);
213+
}
214+
215+
#[test]
216+
fn clamp_page_lowers_page_beyond_last() {
217+
assert_eq!(clamp_page(5, 10, 50), 1);
218+
}
219+
220+
#[test]
221+
fn clamp_page_empty_results_is_first_page() {
222+
assert_eq!(clamp_page(3, 0, 50), 1);
223+
}
224+
225+
#[test]
226+
fn clamp_page_raises_zero_to_first_page() {
227+
assert_eq!(clamp_page(0, 100, 50), 1);
228+
}
229+
200230
// ── SectionRenderer builder ──────────────────────────────────────
201231

202232
#[test]

‎src/utils/vcs.rs‎

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,11 +26,18 @@ pub fn repo_root() -> Result<PathBuf> {
2626
}
2727

2828
/// Extract the `origin` remote's URL from `jj git remote list` output,
29-
/// which prints one `name url` pair per line.
29+
/// which prints one `name url` pair per line. When a remote's fetch and push
30+
/// URLs differ, jj appends a ` (push: <url>)` annotation that is dropped here
31+
/// so only the fetch URL is returned.
3032
fn parse_jj_remote_list(list: &str) -> Option<String> {
3133
list.lines()
3234
.find_map(|line| line.strip_prefix("origin "))
33-
.map(|url| url.trim().to_string())
35+
.map(|url| {
36+
url.split_once(" (push:")
37+
.map_or(url, |(fetch, _)| fetch)
38+
.trim()
39+
.to_string()
40+
})
3441
}
3542

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

217+
#[test]
218+
fn jj_remote_list_strips_push_url_annotation() {
219+
// jj appends `(push: <url>)` when fetch and push URLs differ.
220+
assert_eq!(
221+
parse_jj_remote_list(
222+
"origin https://github.com/usedetail/cli.git (push: https://other.com/cli.git)"
223+
),
224+
Some("https://github.com/usedetail/cli.git".to_string()),
225+
);
226+
}
227+
208228
// ── parse_github_remote_url ─────────────────────────────────────
209229

210230
#[test]
@@ -330,6 +350,15 @@ mod tests {
330350
);
331351
}
332352

353+
#[test]
354+
fn parses_https_with_at_sign_in_password() {
355+
// The credentials/host separator is the last `@`, not the first.
356+
assert_eq!(
357+
parse_github_remote_url("https://user:p@ss@github.com/usedetail/cli.git"),
358+
Some("usedetail/cli".to_string()),
359+
);
360+
}
361+
333362
#[test]
334363
fn parses_http_with_credentials() {
335364
assert_eq!(

0 commit comments

Comments
 (0)