Add pagination to list_users, list_apps, list_tags, list_versions, get_participants#36
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughMultiple MCP tools were changed to add server-side pagination (limit, offset with clamping) and to return a normalized JSON object with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #36 +/- ##
==========================================
+ Coverage 95.92% 95.97% +0.04%
==========================================
Files 24 24
Lines 1889 1911 +22
==========================================
+ Hits 1812 1834 +22
Misses 77 77
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
(AI) Ready for review. All CI checks green. Adds pagination to 5 remaining list tools: list_users (server-side, max 200), list_apps (client-side, max 500), list_tags (client-side, max 500), list_versions (client-side, max 200), get_participants (client-side, max 200). All return standard {data, pagination} format. 79 affected tests pass. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/nc_mcp_server/tools/app_management.py (1)
51-51: Consider defensive handling fordata["apps"]access.Unlike
list_users(which checksisinstance(data, dict) and "users" in data), this directly accessesdata["apps"]. If the OCS API returns an unexpected format, this will raise aKeyError.🛡️ Optional: Add defensive check
- all_apps = data["apps"] + all_apps = data["apps"] if isinstance(data, dict) and "apps" in data else data🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nc_mcp_server/tools/app_management.py` at line 51, Replace the direct indexing of data["apps"] with a defensive check similar to list_users: verify isinstance(data, dict) and "apps" in data before assigning to all_apps (e.g., default to an empty list or log/raise a clear error). Update the code around the all_apps = data["apps"] assignment in app_management.py so it uses the validated value (or fallback) and ensure any downstream code that iterates over all_apps still works with the fallback.tests/integration/test_versions.py (1)
40-45: Consider passinglimitfor consistency.This test omits the
limitparameter while all otherlist_versionscalls in the file uselimit=200. This may be intentional to test default behavior, but it creates inconsistency. If testing defaults is the goal, consider adding a comment to clarify; otherwise, addlimit=200for uniformity.♻️ Suggested change for consistency
`@pytest.mark.asyncio` async def test_returns_json_list(self, nc_mcp: McpTestHelper) -> None: file_id = await _create_versioned_file(nc_mcp, "list-basic") - result = await nc_mcp.call("list_versions", file_id=file_id) + result = await nc_mcp.call("list_versions", file_id=file_id, limit=200) parsed = json.loads(result) assert isinstance(parsed["data"], list)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_versions.py` around lines 40 - 45, The test test_returns_json_list calls nc_mcp.call("list_versions", file_id=file_id) without a limit while other tests pass limit=200; update the call to include limit=200 for consistency (i.e., nc_mcp.call("list_versions", file_id=file_id, limit=200)), referencing the list_versions RPC invocation in this test and the helper _create_versioned_file to locate the test; alternatively, if the intent is to assert default behavior, add a one-line comment above test_returns_json_list clarifying that omission is intentional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/nc_mcp_server/tools/app_management.py`:
- Line 51: Replace the direct indexing of data["apps"] with a defensive check
similar to list_users: verify isinstance(data, dict) and "apps" in data before
assigning to all_apps (e.g., default to an empty list or log/raise a clear
error). Update the code around the all_apps = data["apps"] assignment in
app_management.py so it uses the validated value (or fallback) and ensure any
downstream code that iterates over all_apps still works with the fallback.
In `@tests/integration/test_versions.py`:
- Around line 40-45: The test test_returns_json_list calls
nc_mcp.call("list_versions", file_id=file_id) without a limit while other tests
pass limit=200; update the call to include limit=200 for consistency (i.e.,
nc_mcp.call("list_versions", file_id=file_id, limit=200)), referencing the
list_versions RPC invocation in this test and the helper _create_versioned_file
to locate the test; alternatively, if the intent is to assert default behavior,
add a one-line comment above test_returns_json_list clarifying that omission is
intentional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7bbe2aef-a2ed-4a32-a401-7883dc039e78
📒 Files selected for processing (11)
pyproject.tomlsrc/nc_mcp_server/tools/app_management.pysrc/nc_mcp_server/tools/system_tags.pysrc/nc_mcp_server/tools/talk.pysrc/nc_mcp_server/tools/users.pysrc/nc_mcp_server/tools/versions.pytests/integration/test_app_management.pytests/integration/test_system_tags.pytests/integration/test_talk.pytests/integration/test_users.pytests/integration/test_versions.py
2 participants
Summary
Add limit/offset pagination to 5 remaining list tools that returned unbounded results:
list_userslist_appslist_tagslist_versionsget_participantsAll return the standard
{"data": [...], "pagination": {"count", "offset", "limit", "has_more"}}format.list_usersalso got limit clamping (1-200) — previously passed the limit directly to the OCS API with no validation.Test plan
Summary by CodeRabbit
New Features
Tests
Chores