feat(cdp): implement Browser.setDownloadBehavior file downloads by Ar-maan05 · Pull Request #2722 · lightpanda-io/browser

Ar-maan05 · 2026-06-12T09:23:02Z

Summary

Browser.setDownloadBehavior was a noop (every param commented out, bare success returned), so Lightpanda had no file-download path at all: a response carrying Content-Disposition: attachment was treated as a normal navigation,
its body discarded, downloadPath ignored, and no download events emitted.

This wires up the full path. When Browser.setDownloadBehavior opts in (allow / allowAndName) and a navigation response carries Content-Disposition: attachment, the body is streamed to disk under downloadPath instead of being parsed as a page, and, when eventsEnabled: true, Page.downloadWillBegin and Browser.downloadProgress (inProgress -> completed) are emitted.

Approach

This follows the existing notification pattern: Frame dispatches notifications and the CDP domains consume them and emit CDP events (the same shape as frame_navigated -> Page.frameNavigated).

Session.zig: download config (download_behavior, download_path, download_events_enabled), scoped per CDP connection.
Notification.zig: new DownloadWillBegin / DownloadProgress event
Frame.zig: a .download parse state that holds the open file; detect the attachment in frameHeaderDoneCallback, stream the body to disk in frameDataCallback, close + emit completed in frameDoneCallback, then commit an empty document so the navigation still finishes cleanly (mirrors
the existing .raw path). Content-Disposition parsing handles the quoted, unquoted, and RFC 5987 (filename*=) forms, and strips path components to guard against traversal.
Runner.zig: .download handled as a loading state.
CDP.zig: idempotent registration for the download notifications.
browser.zig / page.zig: setDownloadBehavior now parses and stores
its params and toggles event registration; Browser.downloadProgress and Page.downloadWillBegin emitters.

Behavior notes / scope

Navigation-initiated downloads only. Script-initiated <a download> clicks are the same operation from JS but go through a different entry point; left as a follow-up.
deny falls through to the prior behavior (the response is not written to disk); no cancellation event is emitted.
RFC 5987 filename* values are taken verbatim (no percent-decoding yet).

Testing

New end-to-end CDP test: configures setDownloadBehavior with a temp downloadPath, navigates to a Content-Disposition: attachment response, and asserts both Page.downloadWillBegin and Browser.downloadProgress fire and that the file lands on disk with the correct bytes.
New tests for config storage, registration toggling, and rejection of an unknown behavior.
New unit tests for the Content-Disposition / filename parsing helpers.
Full suite green (make test), formatting clean (zig fmt --check).

Checklist

Tests pass (make test).
Formatting is clean (zig fmt --check ./*.zig ./**/*.zig).
CLA signed.

Ar-maan05 · 2026-06-12T18:22:48Z

Hi, I have really enjoyed contributing to this repo so far. Always happy to help with other open issues if useful.

Ar-maan05 · 2026-06-14T05:41:13Z

I have rebased this branch to the latest main, let me know if any changes are needed.

krichprollsch · 2026-06-16T10:22:42Z

@Ar-maan05 thanks for the PR, It seems that the tests are failing w/ playwright.
You can replay them locally via https://github.com/lightpanda-io/demo/

install deps with npm install
start lightpanda serve
run the test suite go run runner/main.go

Ar-maan05 · 2026-06-16T20:37:57Z

I have fixed the merge conflicts and ensured all the tests pass. CI should be green now. Thank you for the review. Always happy to help with any other issues if useful.

krichprollsch · 2026-06-17T10:02:57Z

+}
+
+// Strips any directory components, guarding against path traversal.
+fn basename(name: []const u8) ?[]const u8 {


Can't we use std.fs.path.basename instead?

Done. Replaced it with std.fs.path.basename (the helper is now sanitizeFilename). I kept a backslash strip on top of it because a Content-Disposition can carry Windows separators even on a Linux host, where the posix basename wouldn't split them, plus the ./../empty guard.

krichprollsch · 2026-06-17T10:07:26Z

+}
+
+// Derives a filename from a URL's last path segment, ignoring query/fragment.
+fn urlBasename(url: []const u8) ?[]const u8 {


I would use a real url parsing instead. see webapi/URL.zig.

Switched to real URL parsing: it now goes through rust-url (sys/url.zig url_parse + url_get_path) and takes the basename of the parsed path, so query/fragment are handled there rather than by hand.

krichprollsch · 2026-06-17T10:17:39Z

+}
+
+// Returns the (optionally quoted) value of `key=` within a parameter list.
+fn paramValue(disposition: []const u8, key: []const u8) ?[]const u8 {


Is it specific to an HTTP header value isn't it?
Maybe we could move it into Header struct from http.zig.
Than you can use directly the Header you get from the response's header iterator.

Maybe you could also implement a values iterator into Header struct, loop hover them and get the value you need directly: attachement, filename, ...

Moved this onto the Header struct in network/http.zig. It now exposes firstValue() (the value before the first ;), param(key), and a params() ParamIterator. dispositionFilename takes the Header straight from the response header iterator and uses .param("filename") / .param("filename*"), so the Content-Disposition-specific string slicing is gone from Frame.

krichprollsch · 2026-06-17T10:26:57Z

+
+    pub fn onDownloadProgress(ctx: *anyopaque, msg: *const Notification.DownloadProgress) !void {
+        const self: *BrowserContext = @ptrCast(@alignCast(ctx));
+        return @import("domains/browser.zig").downloadProgress(self, msg);


I'm not sure I get why you send Browser.downloadProgress and Page.downloadWillBegin and not also Browser.downloadWillBegin + Page.downloadProgress?

Afterall, since we set Browser.setDownloadBehavior I think we should only dispatch Browser.XXX events and not Pages.XXX

Agreed. Since the opt-in is Browser.setDownloadBehavior, I dropped the Page.* events and now emit Browser.downloadWillBegin + Browser.downloadProgress only. The downloadWillBegin emitter moved from page.zig to browser.zig alongside downloadProgress.

krichprollsch · 2026-06-17T10:32:11Z

+// of being parsed as a page, and (when `download_events_enabled` is set)
+// `Page.downloadWillBegin` / `Browser.downloadProgress` events are emitted.
+// `download_path` is duped into the Session arena.
+download_behavior: DownloadBehavior = .default,


I think default is useless, it should be removed and we should set directly deny instead.
And if the CDP command sends a default, then explicitly set the behavior as deny.

Removed the .default variant; the field now defaults to .deny, and a CDP behavior: "default" is explicitly mapped to .deny.

krichprollsch · 2026-06-17T10:44:44Z

-    // })) orelse return error.InvalidParams;
+    const params = (try cmd.params(struct {
+        behavior: []const u8,
+        browserContextId: ?[]const u8 = null,


If browserContextId write a not_implemented log.

if (params.browserContextId != null) { log.warn(.not_implemented, "Browser.setDownloadBehavior", .{ .param = "browserContextId" }); }

Added the not_implemented log as suggested.

krichprollsch · 2026-06-17T10:56:05Z

+        .download => |*download| {
+            download.file.writeAll(data) catch |err| {
+                log.err(.frame, "download write", .{ .err = err, .guid = download.guid });
+                return;


I think we should handle the error here more correctly here, but idk exactly how.
We can't set the parse_state to err directly b/c we could hit the .err => unreachable on next chunck.

So we should either:

change that unreachable on error

set a failure/error on download

And maybe remove the eventually partial file written?

I suggest to write a TODO here for a follow up instead.

Left a TODO here for a follow-up, as you suggested. Noted the two open parts: it can't set parse_state = .err without tripping the .err => unreachable on the next chunk, and it should emit a canceled progress event and remove the partial file.

Ar-maan05 · 2026-06-17T21:53:44Z

@krichprollsch pushed a round addressing all the review comments. Full suite green (make test), zig fmt --check clean.

krichprollsch · 2026-06-18T15:09:35Z

Thanks for the changes, LGMT. I'm preparing a demo/test script for the feature into https://github.com/lightpanda-io/demo/

Ar-maan05 · 2026-06-18T16:21:08Z

Sounds good! Let me know if I can be of any help with this or with any issues/feature implementations.

Browser.setDownloadBehavior was a noop, so Lightpanda had no file-download path. A response with Content-Disposition: attachment is now streamed to disk under downloadPath, and Page.downloadWillBegin / Browser.downloadProgress are emitted when eventsEnabled. Fixes lightpanda-io#2701

…ontext Playwright sends Browser.setDownloadBehavior at the browser level during connection setup, before any target/context exists. Returning an error there aborted the whole connection, failing every playwright demo-runner test. Treat the no-context case as a success no-op, matching the prior behavior.

…eal url/basename, deny default

Ar-maan05 force-pushed the feat-cdp-download-behavior-2701 branch from 3715b14 to 2b22578 Compare June 14, 2026 05:40

krichprollsch self-requested a review June 16, 2026 07:27

krichprollsch reviewed Jun 17, 2026

View reviewed changes

krichprollsch force-pushed the feat-cdp-download-behavior-2701 branch from 0b3e326 to 67f7c89 Compare June 18, 2026 15:05

krichprollsch mentioned this pull request Jun 18, 2026

add a download test lightpanda-io/demo#195

Merged

Ar-maan05 and others added 4 commits June 19, 2026 11:50

address review: Browser-only download events, Header param parsing, r…

ab4d702

…eal url/basename, deny default

cdp: return session id with Browser.setDownloadBehavior

1f85ff1

krichprollsch force-pushed the feat-cdp-download-behavior-2701 branch from 67f7c89 to 1f85ff1 Compare June 19, 2026 09:56

Download events mustn't send session id

0f5ee79

krichprollsch force-pushed the feat-cdp-download-behavior-2701 branch from 5d441ad to 0f5ee79 Compare June 19, 2026 12:31

krichprollsch merged commit a808386 into lightpanda-io:main Jun 19, 2026
40 of 41 checks passed

github-actions Bot locked and limited conversation to collaborators Jun 19, 2026

Uh oh!

Conversation

Ar-maan05 commented Jun 12, 2026

Summary

Approach

Behavior notes / scope

Testing

Checklist

Uh oh!

Ar-maan05 commented Jun 12, 2026

Uh oh!

Ar-maan05 commented Jun 14, 2026

Uh oh!

krichprollsch commented Jun 16, 2026

Uh oh!

Ar-maan05 commented Jun 16, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ar-maan05 commented Jun 17, 2026

Uh oh!

krichprollsch commented Jun 18, 2026

Uh oh!

Ar-maan05 commented Jun 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants