-
Notifications
You must be signed in to change notification settings - Fork 748
feat: add request and response body on network tool #251
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
@microsoft-github-policy-service agree |
I've already tried it on my test products. Thanks to it, I can investigate API calls and order AI to write tests in VSCode only. |
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 like adding the request/response bodies, but I don't think we should limit the tool result to only XHR/fetch requests. Could you trim down the PR to just that? Maybe you could conditionally only include the bodies for XHR/fetch requests to save on some tokens.
@Skn0tt
What do you think about that? Thank you in advance. |
src/tools/network.ts
Outdated
return { | ||
code: [`// <internal code to list network requests>`], | ||
code: ['// <internal code to list all network requests>'], |
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.
revert this line
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 revert as above
src/tools/network.ts
Outdated
@@ -24,15 +24,16 @@ const requests = defineTool({ | |||
|
|||
schema: { | |||
name: 'browser_network_requests', | |||
description: 'Returns all network requests since loading the page', | |||
description: 'Returns all network requests since loading the page, if the request is a xhr or fetch, it will return the request body and response body', |
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 don't think we need to be this explicit here
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 revert this line
src/tools/network.ts
Outdated
if (resourceType === 'xhr' || resourceType === 'fetch') | ||
return true; | ||
const headers = request.headers(); | ||
if (headers['x-requested-with']?.toLowerCase() === 'xmlhttprequest') |
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.
this isn't a standard header, I don't feel comfortable relying on it. please remove this.
src/tools/network.ts
Outdated
return true; | ||
return request.isNavigationRequest() === false && | ||
(headers.accept?.includes('application/json') || | ||
headers['content-type']?.includes('application/json')); |
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.
let's just check if there's a body, not what its content type is. if the request has a body, that's always interesting.
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 fixed the conditions like the below
src/tools/network.ts
Outdated
const headers = request.headers(); | ||
if (headers['x-requested-with']?.toLowerCase() === 'xmlhttprequest') | ||
return true; | ||
return request.isNavigationRequest() === false && |
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.
a form submission could be a navigation request, but still has a super interesting body. let's not check for it being a nav request.
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 fixed the conditions same as the above comment.
- Is application/json request?
- And if there is body, the tool includes it in the result
src/tools/network.ts
Outdated
if (postData) | ||
result.push(`Request Body: ${postData}`); | ||
if (response) { | ||
const body = await response.body(); |
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.
use response.text()
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.
Co-authored-by: Simon Knott <[email protected]>
Co-authored-by: Simon Knott <[email protected]>
Sorry for bothering you. |
I fixed some lint errors and ran |
I am looking for this feature. Can this be available in next release ? |
@praveen2399 Could you please re-review at your earliest convenience? |
That's hardly an excuse to flood the chat with the bodies. I'll close this for now as non-practical. I think we should log network into files (har) and expose them as resources for the next gen clients though. |
Background
Closes #271
Details
network.ts
to enable it to returnrequest
andresponse
body informationxhr
orfetch
request, because the context window is not so big, so big CSS and fonts are a waste of spacenetwork.spec.ts