Repository navigation
Conversation
|
eb0de37 to
d0472fa
Compare
d0472fa to
6b33a7f
Compare
b05d782 to
1bd18a4
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 14 files
Confidence score: 3/5
- For
data:responses innetworkManager.ts,finished()andbody()can hang because the syntheticfinish()only updates network bookkeeping. Ensure it also completes the emittedResponse. - In
networkManager.ts, earlyresponseReceivedExtraInfoevents are lost, soallHeaders()andheadersArray()can return incomplete headers. Cache those events and apply them when theResponseis created.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/extension/understudy/networkManager.ts">
<violation number="1" location="packages/extension/understudy/networkManager.ts:123">
P2: For `data:` responses, the later synthetic `finish()` completes only network bookkeeping; the emitted `Response.finished()` still waits for loading events, so `finished()` and `body()` can hang. Mark the response finished on this synthetic completion.</violation>
<violation number="2" location="packages/extension/understudy/networkManager.ts:331">
P2: `responseReceivedExtraInfo` can arrive before `responseReceived`, but this `Response` is created afterward and has no replay buffer. Cache early extra-info events and apply them so `allHeaders()` and `headersArray()` retain headers such as `set-cookie`.</violation>
</file>
Architecture diagram
sequenceDiagram
participant App as Application Code
participant Page as Page (SDK)
participant RPC as RPC Client
participant RT as StagehandRuntime
participant UPage as Understudy Page
participant NM as NetworkManager
participant CDP as CDP Session
participant Resp as Response Object
participant HT as ResponseHandleTable
Note over App,HT: NEW: Page.onResponse() Event Flow
App->>Page: onResponse(listener)
Page->>Page: Create subscription wrapper
Page->>RPC: subscribe("response", handler)
RPC->>RT: Register subscription (page.event)
RT->>UPage: subscribeResponse(listener)
UPage->>UPage: Wait for page ready
UPage->>NM: subscribeResponse(listener)
NM->>NM: Register onResponse observer
NM-->>UPage: Unsubscribe function
UPage-->>RT: Subscription disposed
RT-->>RPC: Subscription confirmed
RPC-->>Page: CDPSubscription
Page-->>App: Subscription handle
Note over CDP,App: Incoming Response Event
CDP-->>NM: Network.responseReceived
NM->>NM: Check for onResponse observers
alt Has onResponse observer
NM->>Resp: Create Response (lazy body)
NM->>NM: Track pending response
NM->>NM: void response.finished() cleanup
NM->>RT: Emit getResponse event
RT->>HT: register(pageId, response)
HT->>HT: Check WeakMap for existing handle
alt Existing handle for page
HT-->>RT: Return existing responseId
else New response
HT->>HT: Allocate new handle
HT-->>RT: Return new responseId
end
RT->>RT: Build response descriptor
RT-->>UPage: responseDescriptor data
UPage-->>RT: Page event notification
RT-->>RPC: Emit page.event notification
RPC-->>Page: Notification with response data
Page->>Response: Create SDK Response wrapper
Page->>App: Invoke listener(Response)
Note over Resp: Body methods trigger CDP calls
Resp->>CDP: getResponseBody
CDP-->>Resp: Body data
else No onResponse observer
NM->>NM: Skip response emission
end
Note over NM,HT: Response Cleanup
alt Session detached
NM->>NM: disposeResponsesForSession()
NM->>Resp: dispose(Error)
NM->>NM: Remove from pending
else Response finished
Resp->>NM: finished promise resolves
NM->>NM: Remove from pending
end
Note over HT,Resp: Handle Eviction
alt Handle table at capacity
HT->>HT: Remove oldest handle
HT->>Resp: dispose(NotFoundError)
HT->>HT: Clean WeakMap entry
end
Note over RT,HT: Response Disposal
RT->>HT: release(responseId)
HT->>HT: Lookup entry
HT->>HT: Delete from Map
HT->>HT: Delete from WeakMap
HT->>Resp: dispose(ResponseHandleNotFoundError)
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
|
||
| const onResponse = (evt: Protocol.Network.ResponseReceivedEvent) => { | ||
| if (!evt?.requestId) return; | ||
| this.emitResponse(evt, session); |
There was a problem hiding this comment.
P2: For data: responses, the later synthetic finish() completes only network bookkeeping; the emitted Response.finished() still waits for loading events, so finished() and body() can hang. Mark the response finished on this synthetic completion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/extension/understudy/networkManager.ts, line 123:
<comment>For `data:` responses, the later synthetic `finish()` completes only network bookkeeping; the emitted `Response.finished()` still waits for loading events, so `finished()` and `body()` can hang. Mark the response finished on this synthetic completion.</comment>
<file context>
@@ -114,6 +120,7 @@ export class NetworkManager {
const onResponse = (evt: Protocol.Network.ResponseReceivedEvent) => {
if (!evt?.requestId) return;
+ this.emitResponse(evt, session);
const url = evt.response?.url ?? "";
if (url.startsWith("data:")) finish(evt.requestId);
</file context>
There was a problem hiding this comment.
fetch("data:text/plain,Hello"); still triggers Network.loadingFinished event.
| session: CDPSessionLike, | ||
| ): void { | ||
| if (![...this.observers].some((observer) => typeof observer.onResponse === "function")) return; | ||
| const response = new Response({ |
There was a problem hiding this comment.
P2: responseReceivedExtraInfo can arrive before responseReceived, but this Response is created afterward and has no replay buffer. Cache early extra-info events and apply them so allHeaders() and headersArray() retain headers such as set-cookie.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/extension/understudy/networkManager.ts, line 331:
<comment>`responseReceivedExtraInfo` can arrive before `responseReceived`, but this `Response` is created afterward and has no replay buffer. Cache early extra-info events and apply them so `allHeaders()` and `headersArray()` retain headers such as `set-cookie`.</comment>
<file context>
@@ -302,30 +310,66 @@ export class NetworkManager {
+ session: CDPSessionLike,
+ ): void {
+ if (![...this.observers].some((observer) => typeof observer.onResponse === "function")) return;
+ const response = new Response({
+ page: this.page,
+ session,
</file context>
There was a problem hiding this comment.
Thinking we omit Network. responseReceivedExtraInfo as a limitation for now.
| onRequestStarted?(info: NetworkRequestInfo): void; | ||
| onRequestFinished?(info: NetworkRequestInfo): void; | ||
| onRequestFailed?(info: NetworkRequestInfo): void; | ||
| onResponse?(response: Response): void; |
There was a problem hiding this comment.
I don't like a lot of optionals, but better than breaking out NetworkObserver into different types.
| @@ -25,6 +25,7 @@ export class ResponseHandleNotFoundError extends Error { | |||
| * server-side Response object. | |||
| */ | |||
| export class ResponseHandleTable { | |||
There was a problem hiding this comment.
My understanding is that SDK client can operate over many tabs, which means the chromium-supplied request identifier isn't unique can can collide across tabs. So we made this ResponseHandleTable to act as a global lookup.
Might be just me, but I did find our this custom-created responseId a bit confusing as a first time developer. At first, I thought it was referring to the chromium-supplied identifiers. It's not a big deal, but I do wonder if we could avoid creating custom IDs and instead use a compound CDP_SESSION_ID-CDP_NETWORK_REQUEST_ID, which would be globally identifiable and unique across tabs.
Add dedicated
Page.onResponse()event listener, matching Playwright'sPage.on("response")(docs).PR Stack
Page.onResponse#3385Page.onResponse#3386Architecture
There are two broad stroke changes:
Pagethrough -> RPC ->StagehandRuntime-> UnderstudyPage->NetworkManager-> CDP.class Responseas the callback return type! We hold references to these objects in a newResponseHandlerTablethat gets looked up whenever the SDK client asks for it.flowchart TB subgraph CLIENT["SDK client"] PAGE["Page<br/>+onResponse()<br/>+on_response()"] SDKRESP["Response<br/>body() · text() · json()"] RPC["RPCClient"] RPC -.->|"page.event"| PAGE PAGE -->|"listener(response)"| SDKRESP SDKRESP -->|"response.body(responseId)"| RPC end subgraph SW["Extension worker"] RUNTIME["StagehandRuntime<br/>±pageOn()"] HANDLES["+ResponseHandleTable"] RUNTIME -->|"register(response)"| HANDLES HANDLES -.->|"responseId"| RUNTIME RUNTIME -->|"resolve(responseId)"| HANDLES HANDLES -->|"stored Response → body()"| LIVE subgraph UNDERSTUDY["Understudy"] UPAGE["Page<br/>+subscribeResponse()"] NETWORK["NetworkManager<br/>+subscribeResponse()"] LIVE["Response<br/>body() · finished() · loadBody()"] CDP["CdpConnection / CDPSession"] UPAGE -->|"subscribeResponse(listener)"| NETWORK NETWORK -->|"new Response()"| LIVE CDP -->|"Network.responseReceived"| NETWORK LIVE -->|"Network.getResponseBody"| CDP end RUNTIME -->|"subscribeResponse(listener)"| UPAGE end subgraph BROWSER["Chrome browser targets"] TARGETS["Page / child frames"] end RPC -->|"JSON-RPC over CDP"| SW CDP <-->|"CDP"| TARGETS classDef edited fill:#dcfce7,stroke:#16a34a,color:#14532d,stroke-width:2px; classDef existing fill:#f4f4f5,stroke:#a1a1aa,color:#27272a; class PAGE,RUNTIME,HANDLES,UPAGE,NETWORK edited; class SDKRESP,RPC,LIVE,CDP,TARGETS existing;