Skip to content

fix(#124): HAR recording returns 0 entries - add response_body via getResponseBody - #127

Open
lekt9 wants to merge 1 commit into
justrach:mainfrom
lekt9:fix/124-har-response-body
Open

fix(#124): HAR recording returns 0 entries - add response_body via getResponseBody#127
lekt9 wants to merge 1 commit into
justrach:mainfrom
lekt9:fix/124-har-response-body

Conversation

@lekt9

@lekt9 lekt9 commented Mar 31, 2026

Copy link
Copy Markdown

Adds request_id and response_body fields to HarEntry. New fetchResponseBodies helper loops entries after event flush, calls Network.getResponseBody for each, stores body. handleHarStop wires this in before final serialisation. All 17 har+client tests pass.

…seBody

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@justrach justrach left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Good addition — response bodies are the missing piece in our HAR capture. The implementation is clean. A few suggestions:

✅ What's good

  • request_id and response_body fields properly owned/duped/freed
  • fetchResponseBodies() is a clean post-processing step
  • Existing tests updated with new fields

⚠️ Suggestions

  1. Filter response bodies — Currently fetchResponseBodies() fetches bodies for ALL entries indiscriminately. PR #122 had a smarter filter that's worth adopting:

    // Skip non-useful responses
    if (entry.status < 200 or entry.status >= 400) continue;
    if (std.mem.endsWith(u8, entry.url, ".js") or std.mem.endsWith(u8, entry.url, ".css")) continue;
    // Only fetch JSON, HTML, plain text
    if (std.mem.indexOf(u8, entry.mime_type, "json") == null and
        std.mem.indexOf(u8, entry.mime_type, "text/html") == null and
        std.mem.indexOf(u8, entry.mime_type, "text/plain") == null) continue;

    Without filtering, a page with 50+ entries (images, fonts, CSS, JS) will make 50+ serial CDP calls, adding significant latency to /har/stop.

  2. Duplicate doc comments — There are doubled /// Add a manually observed request/response and /// Serialize current entries to HAR 1.2 JSON format. comments in har.zig. Remove the duplicates.

  3. response_body type — You use []const u8 (always allocated). PR #122 used ?[]const u8 (optional) which is cleaner — only allocate when we actually have a body. This avoids allocating empty strings for entries where we skip body fetch.

  4. Rebase neededhar.zig on main now has OwnedEvent struct changes and different field layout. Should be a straightforward rebase.

🔧 After addressing

This is good to merge once the filtering and optional body type are in.

@justrach

Copy link
Copy Markdown
Owner

Can you update this PR to build and test against Zig 0.16.0? The active release/CI path is now targeting 0.16.0, so it would help to rebase and rerun on that toolchain before review.

@justrach

Copy link
Copy Markdown
Owner

Re-reading this against current main:

  • The EventBuffer cross-arena fix here is the right shape — copies events into the long-lived allocator instead of holding a dangling request-arena pointer, with a regression test. Independent of the response-body work.
  • Adding request_id + response_body to HarEntry is the feature half. Two questions before this is mergeable:
    1. Is anything actually calling Network.getResponseBody and threading the result into addEntry? The struct field is added, but I don't see the CDP roundtrip wired up in the diff — without that the field would always be empty. Did I miss it, or is it intentionally a follow-up?
    2. Response bodies can be megabytes; do you want a size cap on what gets pulled, or a --har-body-max-bytes flag?

If you'd rather just land the cross-arena fix now (which is uncontroversial) and split the response-body capture into its own PR, I'd merge the EventBuffer part today.

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.

2 participants