feat(client, server): infinite scroll for tools #1159
Conversation
and1can
force-pushed
the
feat/tools-pagination
branch
from
ff63d94 to
542ea82
Compare
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
dosubot
bot
added
size:L
WalkthroughThis change adds cursor-based infinite scrolling for the tools list: backend and API accept/return an optional cursor; the frontend requests tools with that cursor, appends results, and stores nextCursor. An IntersectionObserver watches a sentinel element to trigger further fetches when the Tools tab is active. Tool execution loading is separated into 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcpjam-inspector/client/src/components/ToolsTab.tsx (1)
265-289: Pagination clears user context and hides the list.fetchToolsalways resets selection/results and the full-screen loading branch is driven byfetchingTools; when infinite scroll loads the next page, the list disappears and selection is lost. Separate “reset” vs “append” behavior, and deriveloadingMoreindependently. Also clear the cursor on a full refresh to avoid starting mid-page.🧭 Suggested adjustment (reset vs append)
- const fetchTools = async () => { + const fetchTools = async (opts?: { reset?: boolean }) => { if (!serverName) { logger.warn("Cannot fetch tools: no serverId available"); return; } - setFetchingTools(true); - setError(""); - setSelectedTool(""); - setFormFields([]); - setResult(null); - setStructuredResult(null); - setShowStructured(false); - setValidationErrors(undefined); - setUnstructuredValidationResult("not_applicable"); + const reset = opts?.reset ?? false; + setFetchingTools(true); + if (reset) { + setCursor(undefined); + setError(""); + setSelectedTool(""); + setFormFields([]); + setResult(null); + setStructuredResult(null); + setShowStructured(false); + setValidationErrors(undefined); + setUnstructuredValidationResult("not_applicable"); + } try { // Call to get all of the tools for server - const data = await listTools(serverName, cursor); + const data = await listTools(serverName, cursor); const toolArray = data.tools ?? []; const dictionary = Object.fromEntries( toolArray.map((tool: Tool) => [tool.name, tool]), ); - setTools((prev) => ({ ...prev, ...dictionary })); + setTools((prev) => (reset ? dictionary : { ...prev, ...dictionary })); setCursor(data.nextCursor); @@ - onRefresh={fetchTools} + onRefresh={() => fetchTools({ reset: true })} @@ - fetchingTools={fetchingTools} + fetchingTools={fetchingTools && !cursor} @@ - loadingMore={fetchingTools} + loadingMore={fetchingTools && !!cursor}Also applies to: 663-667
🤖 Fix all issues with AI agents
In `@mcpjam-inspector/client/src/components/ToolsTab.tsx`:
- Around line 282-284: The call to listTools is passing cursor as the second
parameter (modelId) which breaks pagination; update the call in ToolsTab (where
const data = await listTools(serverName, cursor); and const toolArray =
data.tools ?? []; appear) to pass cursor as the third argument — e.g. await
listTools(serverName, undefined, cursor) (or provide the actual modelId variable
if available) so pagination works and the cursor isn't interpreted as modelId.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mcpjam-inspector/client/src/components/ToolsTab.tsx (2)
265-289: Pagination fetch currently wipes the user’s selection. The reset block runs on every fetch, so loading the next page clears the selected tool and form state. Also, initial fetch can reuse a stale cursor unless you explicitly ignore it for non-append loads.Consider an
appendflag and an explicit cursor choice for the initial page.🔧 Proposed fix (append-aware fetch)
- const fetchTools = async () => { + const fetchTools = async (append = false) => { if (!serverName) { logger.warn("Cannot fetch tools: no serverId available"); return; } setFetchingTools(true); setError(""); - setSelectedTool(""); - setFormFields([]); - setResult(null); - setStructuredResult(null); - setShowStructured(false); - setValidationErrors(undefined); - setUnstructuredValidationResult("not_applicable"); + if (!append) { + setSelectedTool(""); + setFormFields([]); + setResult(null); + setStructuredResult(null); + setShowStructured(false); + setValidationErrors(undefined); + setUnstructuredValidationResult("not_applicable"); + } try { // Call to get all of the tools for server - const data = await listTools(serverName, undefined, cursor); + const effectiveCursor = append ? cursor : undefined; + const data = await listTools(serverName, undefined, effectiveCursor); const toolArray = data.tools ?? []; const dictionary = Object.fromEntries( toolArray.map((tool: Tool) => [tool.name, tool]), ); - setTools((prev) => ({ ...prev, ...dictionary })); + setTools((prev) => (append ? { ...prev, ...dictionary } : dictionary)); setCursor(data.nextCursor); logger.info("Tools fetched", { serverId: serverName, toolCount: toolArray.length, });- fetchTools(); + fetchTools(true);
485-510: AddformFieldsto the useCallback dependencies. The effect correctly identifies the stale closure issue, but the proposed fix omitsformFields—whichbuildParameters()reads. The complete dependency array should be:[ selectedTool, serverName, executeAsTask, taskTtl, serverSupportsTaskToolCalls, selectedToolTaskSupport, + formFields, ]Without it, changing form fields still won't trigger a refresh of the keydown handler.
🤖 Fix all issues with AI agents
In `@mcpjam-inspector/client/src/components/ToolsTab.tsx`:
- Around line 623-625: The code currently passes (activeElicitation.request as
any).requestedSchema directly into the dialog's schema prop which can be a
non-object and cause render errors; update ToolsTab to first read
requestedSchema from activeElicitation.request, validate it is a plain object
(typeof === "object" and not an Array), and only then cast to Record<string,
unknown> (otherwise set undefined) before supplying it to the schema prop so the
dialog always receives a safe object or undefined.
- Around line 588-609: The IntersectionObserver callback in the useEffect for
infinite scroll closes over stale pagination state (cursor and fetchingTools),
so add cursor and fetchingTools (and the fetchTools function reference if it’s
not stable) to the dependency array of the useEffect that references
sentinelRef, activeTab, and filteredToolNames.length; this ensures the observer
is recreated with up-to-date values and prevents repeated fetches after
pagination ends or while a fetch is in progress.
| schema: (activeElicitation.request as any).requestedSchema as | ||
| | Record<string, unknown> | ||
| | undefined, |
There was a problem hiding this comment.
Guard requestedSchema shape before passing to the dialog. Some servers may send non-object schema payloads, which can surface as runtime rendering errors.
🔧 Suggested guard
const requestedSchema = (activeElicitation.request as any).requestedSchema;
const safeSchema =
requestedSchema &&
typeof requestedSchema === "object" &&
!Array.isArray(requestedSchema)
? (requestedSchema as Record<string, unknown>)
: undefined;🤖 Prompt for AI Agents
In `@mcpjam-inspector/client/src/components/ToolsTab.tsx` around lines 623 - 625,
The code currently passes (activeElicitation.request as any).requestedSchema
directly into the dialog's schema prop which can be a non-object and cause
render errors; update ToolsTab to first read requestedSchema from
activeElicitation.request, validate it is a plain object (typeof === "object"
and not an Array), and only then cast to Record<string, unknown> (otherwise set
undefined) before supplying it to the schema prop so the dialog always receives
a safe object or undefined.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mcpjam-inspector/client/src/components/ToolsTab.tsx (2)
203-221: Prevent pagination from wiping UI state or mixing servers.
fetchToolsnow merges results and clears selection every time it runs. When invoked for infinite-scroll or after a server switch/refresh, this can (a) wipe the active tool and results during pagination and (b) append tools from a previous server or reuse a stale cursor. Split “reset” vs “paginate”: reset tools/cursor/UI only for a full reload; paginate should append without clearing.🔧 Suggested fix (split reset vs paginate)
- const fetchTools = async () => { + const fetchTools = async (options: { reset?: boolean } = {}) => { + const { reset = false } = options; if (!serverName) { logger.warn("Cannot fetch tools: no serverId available"); return; } setFetchingTools(true); setError(""); - setSelectedTool(""); - setFormFields([]); - setResult(null); - setStructuredResult(null); - setShowStructured(false); - setValidationErrors(undefined); - setUnstructuredValidationResult("not_applicable"); + if (reset) { + setSelectedTool(""); + setFormFields([]); + setResult(null); + setStructuredResult(null); + setShowStructured(false); + setValidationErrors(undefined); + setUnstructuredValidationResult("not_applicable"); + setTools({}); + setCursor(undefined); + } try { // Call to get all of the tools for server - const data = await listTools(serverName, undefined, cursor); + const data = await listTools( + serverName, + undefined, + reset ? undefined : cursor, + ); const toolArray = data.tools ?? []; const dictionary = Object.fromEntries( toolArray.map((tool: Tool) => [tool.name, tool]), ); - setTools((prev) => ({ ...prev, ...dictionary })); + setTools((prev) => (reset ? dictionary : { ...prev, ...dictionary })); setCursor(data.nextCursor);Also update full reload callers to
fetchTools({ reset: true })(e.g., server change effect and the refresh action).Also applies to: 297-333
516-542: MemoizeexecuteToolwithuseCallbackbefore adding to effect dependencies.The stale closure issue is real: when form fields change, the keydown handler still executes with old parameters because
executeToolcapturesbuildParameters, which readsformFields—but the effect doesn't depend onformFields, so it doesn't re-run.However, adding an unmemoized
executeToolto the dependency array creates a worse problem: the function is redefined on every render, causing the effect to re-run constantly and repeatedly attach/detach the listener.Wrap
executeToolinuseCallbackwith all its captured dependencies (formFields,buildParameters,executeAsTask,selectedToolTaskSupport,taskTtl, etc.), then add the memoized function to the effect deps.
♻️ Duplicate comments (1)
mcpjam-inspector/client/src/components/ToolsTab.tsx (1)
619-625: GuardrequestedSchemabefore passing to the dialog.
Direct casting can surface runtime rendering errors when servers return non-object schemas.🔧 Suggested guard
- const dialogElicitation: DialogElicitation | null = activeElicitation - ? { - requestId: activeElicitation.requestId, - message: activeElicitation.request.message, - schema: (activeElicitation.request as any).requestedSchema as - | Record<string, unknown> - | undefined, - timestamp: activeElicitation.timestamp, - } - : null; + const requestedSchema = (activeElicitation?.request as any)?.requestedSchema; + const safeSchema = + requestedSchema && + typeof requestedSchema === "object" && + !Array.isArray(requestedSchema) + ? (requestedSchema as Record<string, unknown>) + : undefined; + const dialogElicitation: DialogElicitation | null = activeElicitation + ? { + requestId: activeElicitation.requestId, + message: activeElicitation.request.message, + schema: safeSchema, + timestamp: activeElicitation.timestamp, + } + : null;
3 participants
PROBLEM: No pagination for toolsPage. Resolves: #1073
SOLUTION: Adding pagination for toolsPage. Similar pattern as #1157
Note: It was quite hard for me to find a MCP server with pagination (since server sets the pagination page, as per the documentation), so I was not able to test the case with pagination. I was able to test pagination for Resources, and so following the same pattern should also work as expected.