feat(api-web): add "All" pagination option and per-table search filter#3493
Open
kdhulipala-wq wants to merge 2 commits into
Open
feat(api-web): add "All" pagination option and per-table search filter#3493kdhulipala-wq wants to merge 2 commits into
kdhulipala-wq wants to merge 2 commits into
Conversation
Signed-off-by: Krishna Dhulipala <kdhulipala@nvidia.com>
Contributor
Summary by CodeRabbit
WalkthroughAdmin pagination now supports an explicit “All” page across three resource views. The admin interface also serves and loads a client-side table filter with matching-count feedback, keyboard clearing, and associated styling. ChangesAdmin web behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant static_data
participant table_filter.js
participant OverviewTable
Browser->>static_data: request /admin/static/table_filter.js
static_data-->>Browser: return embedded JavaScript
Browser->>table_filter.js: dispatch DOMContentLoaded
table_filter.js->>OverviewTable: attach filter to sortable overview table
Browser->>table_filter.js: enter terms or press Escape
table_filter.js->>OverviewTable: hide rows and update match count
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/api-web/src/compute_allocation.rs`:
- Around line 171-190: Update the pagination offset guards to use >= instead of
>, so an offset equal to the total ID count returns an empty result without
issuing an empty-ID lookup. Apply this in the pagination logic of
crates/api-web/src/compute_allocation.rs (lines 171-190),
crates/api-web/src/instance_type.rs (lines 227-250), and
crates/api-web/src/network_security_group.rs (lines 178-201), preserving the
existing behavior for genuinely in-range pages.
In `@crates/api-web/templates/static/table_filter.js`:
- Around line 56-58: Update the result-count span created in the table filter
code to expose dynamic filter result changes as an appropriate ARIA live region,
while preserving its existing class and text-update behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e3e9cb67-faa3-43a6-99d1-f385b0322b51
📒 Files selected for processing (10)
crates/api-web/src/compute_allocation.rscrates/api-web/src/instance_type.rscrates/api-web/src/lib.rscrates/api-web/src/network_security_group.rscrates/api-web/src/pagination.rscrates/api-web/templates/base.htmlcrates/api-web/templates/pages_footer.htmlcrates/api-web/templates/static/carbide.csscrates/api-web/templates/static/table_filter.jspxe/ipxe/upstream
Comment on lines
+171
to
+190
| // `limit == 0` means "show all" on a single page. | ||
| let (pages, ids_for_page): (usize, Vec<_>) = if limit == 0 { | ||
| (1, all_ids) | ||
| } else { | ||
| let pages = all_ids.len().div_ceil(limit); | ||
| let current_record_cnt_seen = current_page.saturating_mul(limit); | ||
|
|
||
| if current_record_cnt_seen > all_ids.len() { | ||
| return Ok((pages, vec![])); | ||
| } | ||
| if current_record_cnt_seen > all_ids.len() { | ||
| return Ok((pages, vec![])); | ||
| } | ||
|
|
||
| let ids_for_page = all_ids | ||
| .into_iter() | ||
| .skip(current_record_cnt_seen) | ||
| .take(limit) | ||
| .collect(); | ||
| ( | ||
| pages, | ||
| all_ids | ||
| .into_iter() | ||
| .skip(current_record_cnt_seen) | ||
| .take(limit) | ||
| .collect(), | ||
| ) | ||
| }; |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Avoid empty-ID RPCs for the first out-of-range page. When the record count is an exact multiple of limit, current_page == pages makes the offset equal to the ID count. The > check falls through and sends an unnecessary lookup with no IDs; use >=.
crates/api-web/src/compute_allocation.rs#L171-L190: change the offset guard to>=.crates/api-web/src/instance_type.rs#L227-L250: change the offset guard to>=.crates/api-web/src/network_security_group.rs#L178-L201: change the offset guard to>=.
📍 Affects 3 files
crates/api-web/src/compute_allocation.rs#L171-L190(this comment)crates/api-web/src/instance_type.rs#L227-L250crates/api-web/src/network_security_group.rs#L178-L201
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/api-web/src/compute_allocation.rs` around lines 171 - 190, Update the
pagination offset guards to use >= instead of >, so an offset equal to the total
ID count returns an empty result without issuing an empty-ID lookup. Apply this
in the pagination logic of crates/api-web/src/compute_allocation.rs (lines
171-190), crates/api-web/src/instance_type.rs (lines 227-250), and
crates/api-web/src/network_security_group.rs (lines 178-201), preserving the
existing behavior for genuinely in-range pages.
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
Signed-off-by: Krishna Dhulipala <kdhulipala@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-web/src/instance_type.rs (1)
227-252: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd table-driven regression coverage for the pagination boundaries.
Please cover
limit == 0, an exact multiple wherecurrent_page == pages, and pages beyond the end. These cases define the new observable contract and should remain protected by grouped Rust scenarios or equivalent table-driven checks.As per coding guidelines, Rust tests should use table-driven coverage with
scenarios!,value_scenarios!,check_cases, orcheck_values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-web/src/instance_type.rs` around lines 227 - 252, Add grouped table-driven Rust regression tests using the project’s established scenarios!, value_scenarios!, check_cases, or check_values helpers for the pagination logic around limit == 0, an exact multiple with current_page == pages, and pages beyond the end. Assert both the reported page count and returned IDs, including empty results for out-of-range pages, while keeping the cases together as boundary coverage.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/api-web/src/instance_type.rs`:
- Around line 227-252: Add grouped table-driven Rust regression tests using the
project’s established scenarios!, value_scenarios!, check_cases, or check_values
helpers for the pagination logic around limit == 0, an exact multiple with
current_page == pages, and pages beyond the end. Assert both the reported page
count and returned IDs, including empty results for out-of-range pages, while
keeping the cases together as boundary coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 69286c87-0a5f-4363-891b-1e2c1eb322ca
📒 Files selected for processing (4)
crates/api-web/src/compute_allocation.rscrates/api-web/src/instance_type.rscrates/api-web/src/network_security_group.rscrates/api-web/templates/static/table_filter.js
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/api-web/src/compute_allocation.rs
- crates/api-web/templates/static/table_filter.js
- crates/api-web/src/network_security_group.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR restores easy searching of long list pages in the NICo web-UI, which the paginated views had made harder (browser Ctrl-F only sees the current page).
Allchoice (limit=0) alongside 25 / 50 / 100 in the shared pagination footer, so that an entire list renders on one page.limit=0to a default — to honor "All" via a newPageContext::all()helper.table_filter.jsauto-attaches a live filter box above every list table (table.sortable.overview) with 5+ rows. This is case-insensitive, multi-term AND matching, a "Showing X of Y" count, "No matching rows" state, and Esc to clear. Benefits every list screen, including DPU Network Status.Change appears as the folloing:
