-
Notifications
You must be signed in to change notification settings - Fork 48.4k
[mcp] Convert docs resource to tool #33009
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
From what I understand this gives the power to the LLM to browse the docs "on thought" instead of only having them as context? I wonder if there is more fine tuning possible so LLM knows when it should browse react docs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just questions, LGTM!
@@ -87,5 +87,33 @@ export async function queryAlgolia( | |||
}); | |||
const firstResult = results[0] as SearchResponse<DocSearchHit>; | |||
const {hits} = firstResult; | |||
return hits; | |||
const deduped = new Map(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you figure out to remove duplicated paths? Was the tool spouting out duplicated things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the MCP inspector, I noticed it would sometimes return multiple instances of the same url but with a different hash eg https://react.dev/learn/synchronizing-with-effects#what-are-effects-and-how-are-they-different-from-events
and https://react.dev/learn/synchronizing-with-effects#controlling-non-react-widgets
, so I decided to dedupe based on the pathname. We could also have it try to look up just the contents of the anchor instead of return the whole page, but I thought it was just simpler for now to return the whole page.
{ | ||
query: z.string(), | ||
}, | ||
async ({query}) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the LLM coming up with the query here? Like the URL? Or is it defined somewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah the LLM comes up with the query based on a prompt. the user could type something like "look up the latest docs on viewtransitions" and it would call the tool with this query
Seems to work better as a tool. Also it now returns plaintext instead of markdown.
Seems to work better as a tool. Also it now returns plaintext instead of markdown.