-
Notifications
You must be signed in to change notification settings - Fork 0
fix: use per-page fetch for multi-status bugs list queries
#298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5038b23
1c8a94d
e97c5b2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -403,6 +403,42 @@ async fn fetch_all_bugs_multi_status( | |
| Ok(combined) | ||
| } | ||
|
|
||
| /// Fetch a single page of bugs across multiple `statuses`, concatenated in | ||
| /// order and then truncated to `limit` items. | ||
| /// | ||
| /// Each status is queried with the full `limit` and a shared `offset` | ||
| /// derived from `page` so that every status gets a fair chance to | ||
| /// contribute results regardless of how many bugs it contains. The merged | ||
| /// list is then truncated to at most `limit` items, preserving the | ||
| /// page-size contract. | ||
| /// | ||
| /// Unlike `fetch_all_bugs_multi_status` this does NOT exhaust every page — | ||
| /// it issues one bounded request per status and merges the results, keeping | ||
| /// multi-status queries fast even on repos with thousands of bugs. | ||
| async fn fetch_page_multi_status( | ||
| client: &ApiClient, | ||
| repo_id: &RepoId, | ||
| statuses: &[BugReviewState], | ||
| limit: u32, | ||
| page: u32, | ||
| scan_id: Option<&ListPublicBugsWorkflowRequestId>, | ||
| ) -> Result<(Vec<Bug>, usize)> { | ||
| let offset = page_to_offset(page, limit); | ||
| let mut combined = Vec::new(); | ||
| let mut total: usize = 0; | ||
| for status in dedupe_statuses(statuses) { | ||
| let response = client | ||
| .list_bugs(repo_id, status, limit, offset, scan_id) | ||
| .await | ||
| .context("Failed to fetch bugs from repository")?; | ||
| total += usize::try_from(response.total.max(0)).unwrap_or(0); | ||
| combined.extend(response.bugs); | ||
| } | ||
| let limit_usize = usize::try_from(limit).unwrap_or(usize::MAX); | ||
| combined.truncate(limit_usize); | ||
| Ok((combined, total)) | ||
| } | ||
|
Comment on lines
+418
to
+440
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Multi-status paging returns up to
This breaks the pagination contract: JSON consumers see more items than Concrete exampleWith 75 pending + 75 resolved bugs,
The user asked for 50 items per page but gets 100 on page 1. Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 1c8a94d — the limit is now distributed evenly across deduped statuses (with remainder going to the first statuses), so the merged result never exceeds Re: the API contract question — yes, |
||
|
|
||
| pub async fn handle(command: &BugCommands, cli: &crate::Cli) -> Result<()> { | ||
| let client = cli.create_client()?; | ||
|
|
||
|
|
@@ -439,16 +475,17 @@ pub async fn handle(command: &BugCommands, cli: &crate::Cli) -> Result<()> { | |
| let since_ms = resolve_time_flag("--since", since.as_deref(), now)?; | ||
| let until_ms = resolve_time_flag("--until", until.as_deref(), now)?; | ||
|
|
||
| // The bugs API takes a single status per request. When the user | ||
| // asks for several statuses (or `--all`, or any client-side | ||
| // filter), fan out and combine — that also forces the all-fetch | ||
| // path so client-side filters and pagination see the merged set. | ||
| // The bugs API takes a single status per request. When the | ||
| // user asks for client-side filters (`--all`, `--vulns`, | ||
| // `--introduced-by`, `--since`, `--until`) we must fetch every | ||
| // bug to apply them. Multi-status alone does NOT require a full | ||
| // fetch — we can issue one page-sized request per status. | ||
| let needs_full_fetch = *all | ||
| || *vulns | ||
| || !introduced_by.is_empty() | ||
| || since_ms.is_some() | ||
| || until_ms.is_some() | ||
| || status.len() > 1; | ||
| || until_ms.is_some(); | ||
| let multi_status = status.len() > 1; | ||
|
|
||
| if needs_full_fetch { | ||
| let all_bugs = fetch_all_bugs_multi_status( | ||
|
|
@@ -495,6 +532,19 @@ pub async fn handle(command: &BugCommands, cli: &crate::Cli) -> Result<()> { | |
| } | ||
| 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. | ||
| let (bugs, total) = fetch_page_multi_status( | ||
| &client, | ||
| &resolved_repo_id, | ||
| status, | ||
| *limit, | ||
| *page, | ||
| scan_id.as_ref(), | ||
| ) | ||
| .await?; | ||
| output_list(&bugs, total, *page, *limit, format) | ||
|
Comment on lines
+535
to
+547
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📝 Info: The The single-status path ( Was this helpful? React with 👍 or 👎 to provide feedback.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct — this is intentional and consistent with the single-status path, which also doesn't print hints. Hints are only relevant when client-side filters are active (which routes through There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: The new multi-status fast path uses per-status paging but still reports pagination using single-list Prompt for AI agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was addressed in 1c8a94d — the limit is now distributed evenly across deduped statuses (with remainder to the earliest), so the merged result never exceeds |
||
| } else { | ||
| // Single-status, no other filters: keep the original | ||
| // single-page server fetch — cheaper and lets the API drive | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚩 Summed
totalacross statuses may mislead pagination metadata even if page size is fixedBeyond the items-per-page issue (reported as a bug), note that summing
totalfrom independent per-status API responses (src/commands/bugs.rs:427) produces a correct grand total only if the API returns the total count for the given status, not a global count. This is almost certainly the case (the API filters by status), but worth verifying against the API contract. If the API ever returned the unfiltered total, the summedtotalwould be inflated bynum_statuses×.Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed — the API returns per-status totals (the
statusparameter is a required filter onlist_public_bugs). The sum across statuses gives the correct grand total. Addressed the page-size issue in 1c8a94d by distributing the limit across statuses.