Skip to content

Fix Request/Response correlation: implement requestId-based tracking in playwright-adapter#39

Draft
Copilot wants to merge 4 commits into
mainfrom
copilot/fix-request-response-correlation
Draft

Fix Request/Response correlation: implement requestId-based tracking in playwright-adapter#39
Copilot wants to merge 4 commits into
mainfrom
copilot/fix-request-response-correlation

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Nov 14, 2025

Network event handlers (onRequest, onRequestFinished, onRequestFailed, onResponse) were creating new RequestImpl instances per event, breaking object identity semantics. ResponseImpl.request() always returned null.

Changes

PageImpl: Request lifecycle management

  • Added Map<String, RequestImpl> requestsById keyed by BiDi requestId
  • onRequest: creates and caches RequestImpl by requestId
  • onRequestFinished/onRequestFailed: retrieves cached instance, enriches with response/error data, removes from map
  • onResponse: passes cached RequestImpl to ResponseImpl constructor
  • Added extractRequestId(WDBaseParameters) helper

RequestImpl: Mutable enrichment

  • Changed failure and response from final to mutable
  • Added enrichWithResponse(ResponseCompleted) and enrichWithError(FetchError) public methods

ResponseImpl: Request reference

  • Constructor now accepts Request request parameter
  • request() returns the provided Request instance

Result

All events for a logical request now share the same RequestImpl instance:

final Set<Request> inFlight = new HashSet<>();
page.onRequest(r -> inFlight.add(r));
page.onRequestFinished(r -> inFlight.remove(r));  // ✓ same object, removal works
page.onResponse(resp -> {
    assert resp.request() != null;  // ✓ returns correct Request
});

Enables robust in-flight request tracking, waitForNetworkIdle implementation, and proper Playwright network API semantics.

Testing

Added RequestResponseCorrelationTest with 5 unit tests covering enrichment methods, object identity, and edge cases. All tests passing. CodeQL: 0 vulnerabilities.

Original prompt

This section details on the original issue you should resolve

<issue_title>Playwright adapter: Request/Response correlation broken (RequestImpl instances + ResponseImpl.request())</issue_title>
<issue_description>## Severity
High priority: This directly affects correct behavior of Playwright-style network APIs and makes it impossible to reliably correlate requests and responses from the TestRunner (and anywhere else that uses onRequest, onRequestFinished, onRequestFailed, or onResponse).

Problem

In the current playwright-adapter implementation:

  1. PageImpl.onRequest / onRequestFinished / onRequestFailed each create a new RequestImpl

    • onRequest (network.beforeRequestSent) → new RequestImpl(WDNetworkEvent.BeforeRequestSent)
    • onRequestFinished (network.responseCompleted) → new RequestImpl(WDNetworkEvent.ResponseCompleted)
    • onRequestFailed (network.fetchError) → new RequestImpl(WDNetworkEvent.FetchError)
    • There is no shared RequestImpl per real network request, and no map keyed by requestId.
  2. ResponseImpl.request() always returns null

    • In ResponseImpl the request field is never set (constructor leaves it as null).
    • As a result, response.request() from page.onResponse(...) cannot be used to correlate back to Request.
  3. There is no internal mapping from BiDi requestIdRequestImpl that would allow Playwright semantics to be respected.

This is not compatible with the official Playwright semantics, where all events for a single underlying browser request operate on the same Request object, and response.request() returns that same instance.

Why this matters

For features like:

  • Tracking in-flight network activity in the TestRunner (e.g. to pause test execution while a transfer is in progress, including navigation or AJAX),
  • Implementing waitForNetworkIdle-like behavior purely on top of Playwright APIs (onRequest, onRequestFinished, onRequestFailed, onResponse),
  • Any advanced network inspection or per-request lifecycle logging,

we need to be able to rely on:

  • Request identity being stable across events (onRequestonRequestFinished / onRequestFailed), and
  • Response.request() returning that same Request instance.

Derzeit ist das nicht gegeben, wodurch z. B. ein Counter im TestRunner, der Requests aus onRequest merkt und in onRequestFinished/onRequestFailed wieder entfernt, nicht sicher mit Objektidentität arbeiten kann. Außerdem ist response.request() derzeit unbrauchbar.

Expected behavior (aligned with Playwright API)

For each real network request:

  1. There is exactly one RequestImpl instance representing it.
  2. page.onRequest(handler) is called with that instance when the request starts.
  3. page.onRequestFinished(handler) (and page.onRequestFailed(handler)) are called with the same RequestImpl instance when the request completes or fails.
  4. page.onResponse(handler) receives a ResponseImpl whose request() method returns the same RequestImpl instance.

Suggested implementation approach

  1. Introduce a Map<String, RequestImpl> keyed by BiDi requestId in the adapter (likely in BrowserImpl or PageImpl, wherever network events are routed):

    • On BeforeRequestSent:

      • Extract requestId from the BiDi event.
      • Create RequestImpl once: RequestImpl req = new RequestImpl(beforeRequestSent).
      • Put it into requestsById under that requestId.
      • Invoke all registered onRequest handlers with req.
    • On ResponseCompleted:

      • Extract requestId.
      • Look up RequestImpl req = requestsById.get(requestId).
      • If req is null (edge case), create it and store it, but ideal is to re-use the existing.
      • Enrich req with response data (rather than building a new RequestImpl).
      • Call onRequestFinished with this req.
      • Optionally, remove requestId from the map once the lifecycle is finished.
    • On FetchError:

      • Same pattern: lookup by requestId, enrich req with error data, call onRequestFailed with the same instance.
  2. In ResponseImpl:

    • Also key by requestId and reuse the RequestImpl from requestsById.
    • Set the request field in the constructor so that response.request() returns the correct, shared RequestImpl.
  3. Ensure that offRequest, offRequestFinished, offRequestFailed, offResponse in PageImpl still work by keeping the {handler -> adapter} maps you already have, but the objects passed to handlers should be the shared RequestImpl/ResponseImpl instances from the maps.

Acceptance criteria

  • onRequest, onRequestFinished, onRequestFailed and onResponse for a given logical request all see the same Request object (identity stable for that request).

  • ResponseImpl.request() returns a non-null Request that equals the one from onRequest.

  • A simple test like:

    
    

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 3 commits November 14, 2025 04:05
Co-authored-by: Miguel0888 <40050873+Miguel0888@users.noreply.github.com>
Co-authored-by: Miguel0888 <40050873+Miguel0888@users.noreply.github.com>
Co-authored-by: Miguel0888 <40050873+Miguel0888@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix request/response correlation in Playwright adapter Fix Request/Response correlation: implement requestId-based tracking in playwright-adapter Nov 14, 2025
Copilot AI requested a review from Miguel0888 November 14, 2025 04:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Playwright adapter: Request/Response correlation broken (RequestImpl instances + ResponseImpl.request())

2 participants