fix(mcp): default-limit and paginate item(list) - #358
Conversation
item(list) had no cap when limit was omitted, unlike search/groom, so a bare call on a large project could overflow the MCP response token limit (155 items / 52k chars observed). Default it to 50 (capped at 500) and return a pagination envelope (total/offset/limit/next_offset/prev_offset) so callers can page forward/backward without re-deriving offsets by hand.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesItem list pagination
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant item_list
participant ItemListPage
Caller->>item_list: Send list request with limit and offset
item_list->>ItemListPage: Build page with items and navigation metadata
ItemListPage-->>Caller: Return serialized paginated response
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/mcp_server/tests/item_tests.rs (1)
1250-1261: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover omitted and capped limits.
This test validates explicit
limit = 1, but not the core new behaviors: omittedlimitdefaults to 50 and values above 500 are capped. Add cases with more than 50 and more than 500 matching items.🤖 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 `@src/mcp_server/tests/item_tests.rs` around lines 1250 - 1261, Add test cases alongside the existing item-listing assertions to cover omitted limit behavior with more than 50 matching items and an explicit limit above 500 with more than 500 matches. Verify the omitted limit returns at most 50 items and the oversized limit returns at most 500, while preserving the existing pagination assertions for explicit limit = 1.
🤖 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 `@src/mcp_server/item.rs`:
- Around line 294-295: Update prev_offset in the pagination logic to derive the
previous page from min(offset, total), ensuring out-of-range offsets navigate
back toward the valid range. Also suppress prev_offset when limit == 0, while
preserving the existing saturating subtraction behavior for valid positive
limits.
---
Nitpick comments:
In `@src/mcp_server/tests/item_tests.rs`:
- Around line 1250-1261: Add test cases alongside the existing item-listing
assertions to cover omitted limit behavior with more than 50 matching items and
an explicit limit above 500 with more than 500 matches. Verify the omitted limit
returns at most 50 items and the oversized limit returns at most 500, while
preserving the existing pagination assertions for explicit limit = 1.
🪄 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: Pro Plus
Run ID: a2c7fc3c-2b75-456c-a61e-bd9e67678117
📒 Files selected for processing (3)
src/mcp_server/item.rssrc/mcp_server/tests/item_tests.rssrc/mcp_server/types.rs
…inding - cargo fmt wrap on the next_offset line (CI fmt check was failing) - prev_offset now clamps via offset.min(total) so an out-of-range offset navigates back into range instead of repeating an empty page, and both next_offset/prev_offset suppress when limit == 0
Summary
item(list)had no cap on results whenlimitwas omitted, unlikesearch/groom— a bare call on a large project (155 items) overflowed the MCP response token limit (52,194 chars).limitto 50 (capped at 500), matching the existingunwrap_or+clamppattern used bysearch/groom.listnow returns a pagination envelope{items, total, offset, limit, next_offset, prev_offset}instead of a bare array, so callers can page forward/backward by passingnext_offset/prev_offsetstraight back asoffset— no manual math.limit/offsetfield schema descriptions accordingly.Test plan
cargo test -p agentflare item_tests— 35 passed (including new assertions ontotal/next_offset/prev_offset)cargo build -p agentflare --binscargo clippy -p agentflare --bins -- -D warningsSummary by CodeRabbit
New Features
items,total,offset,limit, and navigation metadata for next/previous pages.Bug Fixes