feat(registry): add pagination to GET /agents endpoint - #95
Merged
Bosun-Josh121 merged 4 commits intoJul 29, 2026
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe registry’s ChangesRegistry pagination
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GETAgents
participant AgentStore
Client->>GETAgents: Request agents with filters and pagination
GETAgents->>AgentStore: Load matching agents
AgentStore-->>GETAgents: Filtered agent records
GETAgents-->>Client: Ordered page with total, limit, and offset
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/development.md`:
- Around line 181-186: Update the GET /agents examples in docs/development.md so
the paginated first-page request explicitly uses ?limit=20, and revise the
accompanying rationale to state that the maximum applies only when pagination
parameters are provided; preserve the offset example for agents 21–40.
In `@packages/registry/src/server.test.ts`:
- Around line 407-490: Update the GET /agents tests to seed agents through the
store and invoke the HTTP route rather than applying sorting, filtering, or
pagination directly to loadAgents(). Assert each response body shape and values,
including backward-compatible bare arrays, pagination totals, limit/offset
clamping, and the invalid offset=abc case.
In `@packages/registry/src/server.ts`:
- Around line 114-115: Update the offset parsing in the server request handler
so non-numeric values such as “abc” resolve to 0 before calculating
clampedOffset. Preserve the existing behavior for valid offsets and ensure the
serialized offset and pagination logic use the resulting zero value instead of
NaN.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b8777c1d-9091-49c4-85c5-375f5eae8cfc
📒 Files selected for processing (3)
docs/development.mdpackages/registry/src/server.test.tspackages/registry/src/server.ts
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 `@packages/registry/src/server.test.ts`:
- Around line 416-422: Update the test case “applies default limit when only
limit param key is provided” to request pagination without a limit parameter,
such as using only ?offset=0, so it exercises the default-limit path. Keep the
assertions verifying a 20-item limit and the existing response and agent-count
expectations.
- Around line 454-460: Update the limit parsing and clamping logic exercised by
the `/agents` handler to distinguish missing or non-numeric values from numeric
zero, preserving the default only for invalid or absent input and clamping
`limit=0` to the minimum value of 1 within the declared 1–100 range. Revise the
test case to expect a response limit of 1 instead of 20.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 10806549-a975-450b-9d5b-e7bf75c642d1
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
docs/development.mdpackage.jsonpackages/registry/src/server.test.tspackages/registry/src/server.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/registry/src/server.ts
- docs/development.md
Comment on lines
+454
to
+460
| it('clamps limit to minimum (1) when limit is 0 — falls back to default', async () => { | ||
| // limit=0 is parsed as 0, which is falsy, so the server substitutes DEFAULT_LIMIT (20) | ||
| const res = await request(app).get('/agents?limit=0'); | ||
| expect(res.status).toBe(200); | ||
| expect(res.body.limit).toBe(20); // default limit applied when limit resolves to 0 | ||
| expect(Array.isArray(res.body.agents)).toBe(true); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clamp numeric zero to the minimum of 1.
This test enshrines limit=0 → 20, contradicting the PR’s declared 1..100 clamp range. The handler’s falsy fallback has the same defect; distinguish an absent/non-numeric value from numeric zero, then clamp zero to 1.
Proposed test adjustment
- it('clamps limit to minimum (1) when limit is 0 — falls back to default', async () => {
- // limit=0 is parsed as 0, which is falsy, so the server substitutes DEFAULT_LIMIT (20)
+ it('clamps limit to minimum (1) when limit is 0', async () => {
const res = await request(app).get('/agents?limit=0');
expect(res.status).toBe(200);
- expect(res.body.limit).toBe(20); // default limit applied when limit resolves to 0
+ expect(res.body.limit).toBe(1);
expect(Array.isArray(res.body.agents)).toBe(true);
+ expect(res.body.agents).toHaveLength(1);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('clamps limit to minimum (1) when limit is 0 — falls back to default', async () => { | |
| // limit=0 is parsed as 0, which is falsy, so the server substitutes DEFAULT_LIMIT (20) | |
| const res = await request(app).get('/agents?limit=0'); | |
| expect(res.status).toBe(200); | |
| expect(res.body.limit).toBe(20); // default limit applied when limit resolves to 0 | |
| expect(Array.isArray(res.body.agents)).toBe(true); | |
| }); | |
| it('clamps limit to minimum (1) when limit is 0', async () => { | |
| const res = await request(app).get('/agents?limit=0'); | |
| expect(res.status).toBe(200); | |
| expect(res.body.limit).toBe(1); | |
| expect(Array.isArray(res.body.agents)).toBe(true); | |
| expect(res.body.agents).toHaveLength(1); | |
| }); |
🤖 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 `@packages/registry/src/server.test.ts` around lines 454 - 460, Update the
limit parsing and clamping logic exercised by the `/agents` handler to
distinguish missing or non-numeric values from numeric zero, preserving the
default only for invalid or absent input and clamping `limit=0` to the minimum
value of 1 within the declared 1–100 range. Revise the test case to expect a
response limit of 1 instead of 20.
Distinguish numeric zero from invalid/absent values in limit parsing. Update tests to verify default limit behavior and zero clamping.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/registry/src/server.test.ts (1)
321-491: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a filter-plus-pagination integration test.
The suite covers pagination mechanics but does not verify pagination after an existing filter. Add a request such as
GET /agents?status=active&limit=1&offset=1and assert both the page contents and post-filtertotal; this catches regressions that slice before filtering or report the unfiltered count.🤖 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 `@packages/registry/src/server.test.ts` around lines 321 - 491, Add an integration test within the GET /agents pagination describe block that requests status=active with limit=1 and offset=1, then assert the response contains the correct second filtered agent and reports the post-filter total of 3. Verify the paginated envelope and status code, ensuring filtering occurs before slicing and total calculation.
🤖 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.
Outside diff comments:
In `@packages/registry/src/server.test.ts`:
- Around line 321-491: Add an integration test within the GET /agents pagination
describe block that requests status=active with limit=1 and offset=1, then
assert the response contains the correct second filtered agent and reports the
post-filter total of 3. Verify the paginated envelope and status code, ensuring
filtering occurs before slicing and total calculation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e596c4c-8ae8-41d1-b3b7-edacf24edc83
📒 Files selected for processing (2)
packages/registry/src/server.test.tspackages/registry/src/server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/registry/src/server.ts
Collaborator
|
@Oluwasuyi-Oluwatimilehin-Daniel please fix CI errors |
Contributor
Author
|
@Bosun-Josh121 i already fixed ci |
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
2 participants
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.
#closes #86
Add pagination to GET /agents in registry
Summary
Implements pagination for the GET /agents endpoint to handle unbounded responses as the registry grows. The endpoint now supports
limitandoffsetquery parameters with sensible defaults and clamping.Changes
Key Features
{agents, total, limit, offset}Testing
API Change
When
limitoroffsetare provided, response format changes from bare array to envelope. Existing clients without pagination params continue to work unchanged.#closes
Summary by CodeRabbit
GET /agentsendpoint vialimitandoffset.agents,total,limit, andoffsetwhen it is.GET /agentsdocumentation with parameters, response formats, and examples.