Skip to content

Commit 3a94c6e

Browse files
fix(preview): keep desktop-rendered tabs in step with the server
Review found gaps in how the desktop hands its native tabs to the server: - Closing a tab never told the server it detached: the close took the tab out of the map before the detach looked for it. - A restarted backend never heard about tabs already attached. The announcement read the tab list when the host was built, and ran before the new backend subscribed. Each subscription now announces itself. - A reply from a relay the server released could reach the next connection and answer a request id it reused. Only the current relay writes now. - An endpoint for a tab that detached before its queue registered handed Playwright a URL that answered nothing until it timed out. - Downloads in these tabs were lost. Removing the desktop's automation took its download handler with it, and the relay only pretended to accept Browser.setDownloadBehavior. The relay now applies it and forwards the download events, and the desktop saves the file under the CDP guid where the server's engine reads it, as Chromium does for a headless tab. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 parent d9ee6f3 commit 3a94c6e

9 files changed

Lines changed: 372 additions & 24 deletions

File tree

‎apps/desktop/src/backend/DesktopBackendManager.ts‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -947,10 +947,7 @@ export const makeBackendInstance = Effect.fn("makeBackendInstance")(function* (
947947
...config.value,
948948
desktopTelemetryStream: desktopTelemetryPublisher.encoded,
949949
// Only a bootstrap that names the browser fds (the local primary) gets them.
950-
desktopBrowserStream: Stream.concat(
951-
Stream.fromEffectDrain(desktopBrowserHost.announceAll),
952-
desktopBrowserHost.events,
953-
),
950+
desktopBrowserStream: desktopBrowserHost.events,
954951
onDesktopBrowserCommand: desktopBrowserHost.handleCommandLine,
955952
onDesktopTelemetryControl: (message) =>
956953
desktopTelemetryPublisher.handleControlForSource(spec.id, message),

‎apps/desktop/src/preview/CdpRelay.test.ts‎

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,16 @@ import { describe, expect, it, vi } from "vite-plus/test";
22

33
import { createCdpRelayConnection, type CdpRelayTarget } from "./CdpRelay.ts";
44

5-
const makeTarget = (send: CdpRelayTarget["send"]): CdpRelayTarget => ({
5+
const makeTarget = (
6+
send: CdpRelayTarget["send"],
7+
setDownloadDirectory: CdpRelayTarget["setDownloadDirectory"] = () => {},
8+
): CdpRelayTarget => ({
69
send,
710
targetId: async () => "GUEST-TARGET",
811
url: () => "http://localhost:4719/",
912
title: () => "Fixture",
1013
userAgent: () => "Electron",
14+
setDownloadDirectory,
1115
});
1216

1317
// Each relay reply resolves after a few microtasks; one macrotask lets them all land.
@@ -130,3 +134,41 @@ describe("CDP relay extra sessions", () => {
130134
expect(written.map((m) => m["sessionId"])).toEqual(["t3-preview-page", extra]);
131135
});
132136
});
137+
138+
describe("CDP relay downloads", () => {
139+
it("applies the server's download directory and reports downloads on the root session", async () => {
140+
const send = vi.fn(async () => ({}));
141+
const directories: Array<string | null> = [];
142+
const written: Array<Record<string, unknown>> = [];
143+
const relay = createCdpRelayConnection(
144+
makeTarget(send, (directory) => directories.push(directory)),
145+
(raw) => written.push(JSON.parse(raw)),
146+
);
147+
relay.receive(JSON.stringify({ id: 1, method: "Target.setAutoAttach", params: {} }));
148+
relay.receive(
149+
JSON.stringify({
150+
id: 2,
151+
method: "Browser.setDownloadBehavior",
152+
params: {
153+
behavior: "allowAndName",
154+
browserContextId: "t3-preview",
155+
downloadPath: "/srv/artifacts",
156+
eventsEnabled: true,
157+
},
158+
}),
159+
);
160+
await settle();
161+
expect(directories).toEqual(["/srv/artifacts"]);
162+
// The tab's own debugger has no such context, so the id stays here.
163+
expect(send).toHaveBeenCalledWith(
164+
"Browser.setDownloadBehavior",
165+
{ behavior: "allowAndName", downloadPath: "/srv/artifacts", eventsEnabled: true },
166+
undefined,
167+
);
168+
written.length = 0;
169+
relay.event("Browser.downloadWillBegin", { guid: "g1", suggestedFilename: "r.csv" }, "");
170+
expect(written).toEqual([
171+
{ method: "Browser.downloadWillBegin", params: { guid: "g1", suggestedFilename: "r.csv" } },
172+
]);
173+
});
174+
});

‎apps/desktop/src/preview/CdpRelay.ts‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,12 @@ export interface CdpRelayTarget {
2121
readonly url: () => string;
2222
readonly title: () => string;
2323
readonly userAgent: () => string;
24+
/**
25+
* Where the server wants this tab's downloads, named by their CDP guid. The
26+
* desktop app and the server it launched share a disk, so the server's
27+
* Playwright reads the file where it asked for it.
28+
*/
29+
readonly setDownloadDirectory: (directory: string | null) => void;
2430
}
2531

2632
export interface CdpRelayConnection {
@@ -52,7 +58,6 @@ const PAGE_SESSION_ID = "t3-preview-page";
5258
/** Browser-level commands that only need acknowledging for a page the desktop owns. */
5359
const ACKNOWLEDGED = new Set([
5460
"Target.setDiscoverTargets",
55-
"Browser.setDownloadBehavior",
5661
"Browser.grantPermissions",
5762
"Browser.resetPermissions",
5863
"Browser.cancelDownload",
@@ -89,6 +94,15 @@ export function createCdpRelayConnection(
8994
const browserCommand = async (command: CdpCommand): Promise<unknown> => {
9095
if (ACKNOWLEDGED.has(command.method)) return {};
9196
switch (command.method) {
97+
case "Browser.setDownloadBehavior": {
98+
// The tab's debugger reports downloads as Chromium does, so the server
99+
// sees them as its own. Only the directory needs the desktop's help.
100+
const directory = command.params?.["downloadPath"];
101+
const allowed = command.params?.["behavior"] !== "deny" && typeof directory === "string";
102+
target.setDownloadDirectory(allowed ? directory : null);
103+
const { browserContextId: _ignored, ...params } = command.params ?? {};
104+
return target.send("Browser.setDownloadBehavior", params, undefined);
105+
}
92106
case "Browser.getVersion":
93107
return {
94108
protocolVersion: "1.3",
@@ -188,6 +202,11 @@ export function createCdpRelayConnection(
188202
},
189203
event: (method, params, sessionId) => {
190204
if (!attached) return;
205+
// Download events are the browser's; Playwright listens on its root session.
206+
if (method.startsWith("Browser.download")) {
207+
send({ method, params });
208+
return;
209+
}
191210
// Electron reports the page's own events with an empty session id.
192211
if (sessionId) {
193212
send({ method, params, sessionId });
Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
// @effect-diagnostics nodeBuiltinImport:off - Stands in for an Electron debugger.
2+
import { describe, expect, it } from "@effect/vitest";
3+
import { DesktopBrowserEvent } from "@t3tools/contracts";
4+
import * as Effect from "effect/Effect";
5+
import * as Fiber from "effect/Fiber";
6+
import * as Schema from "effect/Schema";
7+
import * as Stream from "effect/Stream";
8+
import * as NodeEvents from "node:events";
9+
10+
import * as DesktopBrowserHost from "./DesktopBrowserHost.ts";
11+
12+
const decodeEvent = Schema.decodeUnknownSync(Schema.fromJsonString(DesktopBrowserEvent));
13+
const encodeJson = Schema.encodeSync(Schema.fromJsonString(Schema.Unknown));
14+
const decodeCdpReply = Schema.decodeUnknownSync(
15+
Schema.fromJsonString(Schema.Struct({ id: Schema.Number })),
16+
);
17+
const key = { threadId: "thread-1", tabId: "tab-1" };
18+
19+
/** A tab's webContents and debugger, with the debugger's commands left pending until released. */
20+
const makeDebuggee = () => {
21+
const emitter = new NodeEvents.EventEmitter();
22+
const pending: Array<() => void> = [];
23+
const debuggee = Object.assign(emitter, {
24+
sendCommand: (method: string) =>
25+
new Promise((resolve) => {
26+
if (method === "Target.getTargetInfo") {
27+
resolve({ targetInfo: { targetId: "GUEST" } });
28+
return;
29+
}
30+
pending.push(() => resolve({ method }));
31+
}),
32+
});
33+
const webContents = {
34+
getURL: () => "http://localhost/",
35+
getTitle: () => "Page",
36+
getUserAgent: () => "Electron",
37+
};
38+
return {
39+
tab: {
40+
webContents: webContents as unknown as Electron.WebContents,
41+
debugger: debuggee as unknown as Electron.Debugger,
42+
},
43+
emit: (method: string, params: unknown) => emitter.emit("message", {}, method, params, ""),
44+
release: () => pending.splice(0).forEach((resolve) => resolve()),
45+
};
46+
};
47+
48+
/** Reads `count` events from one backend's subscription. */
49+
const takeEvents = (host: DesktopBrowserHost.DesktopBrowserHost["Service"], count: number) =>
50+
host.events.pipe(
51+
Stream.take(count),
52+
Stream.runCollect,
53+
Effect.map((lines) => lines.map((line) => decodeEvent(new TextDecoder().decode(line)))),
54+
);
55+
56+
describe("DesktopBrowserHost", () => {
57+
it.effect("announces tabs already attached to a backend that starts later", () =>
58+
Effect.gen(function* () {
59+
const host = yield* DesktopBrowserHost.make;
60+
host.attach(key, makeDebuggee().tab);
61+
// A restarted backend subscribes after the attach and still hears it.
62+
expect(yield* takeEvents(host, 1)).toEqual([{ type: "attached", ...key }]);
63+
expect(yield* takeEvents(host, 1)).toEqual([{ type: "attached", ...key }]);
64+
}),
65+
);
66+
67+
it.effect("drops replies from a relay the server released", () =>
68+
Effect.gen(function* () {
69+
const host = yield* DesktopBrowserHost.make;
70+
const debuggee = makeDebuggee();
71+
host.attach(key, debuggee.tab);
72+
const reader = yield* takeEvents(host, 2).pipe(Effect.forkScoped);
73+
// Wait until the reader has received the announcement.
74+
yield* Effect.yieldNow;
75+
const command = (id: number, method: string) =>
76+
host.handleCommandLine(
77+
encodeJson({
78+
type: "cdp",
79+
...key,
80+
message: encodeJson({ id, method, sessionId: "t3-preview-page" }),
81+
}),
82+
);
83+
yield* command(1, "Page.captureScreenshot");
84+
yield* host.handleCommandLine(encodeJson({ type: "release", ...key }));
85+
yield* command(2, "DOM.enable");
86+
debuggee.release();
87+
yield* Effect.promise(() => new Promise((resolve) => setImmediate(resolve)));
88+
const [, reply] = yield* Fiber.join(reader);
89+
// Only the new connection's reply arrives; the old one's id could collide.
90+
expect(reply).toMatchObject({ type: "cdp" });
91+
expect(decodeCdpReply((reply as { message: string }).message).id).toBe(2);
92+
}).pipe(Effect.scoped),
93+
);
94+
95+
it.effect("saves a server tab's download under its CDP guid where the server asked", () =>
96+
Effect.gen(function* () {
97+
const host = yield* DesktopBrowserHost.make;
98+
const debuggee = makeDebuggee();
99+
host.attach(key, debuggee.tab);
100+
const paths: Array<string> = [];
101+
const item = {
102+
setSavePath: (path: string) => void paths.push(path),
103+
} as unknown as Electron.DownloadItem;
104+
// Before the server sets a directory, Electron keeps its own handling.
105+
expect(host.placeDownload(debuggee.tab.webContents, item)).toBe(false);
106+
yield* host.handleCommandLine(
107+
encodeJson({
108+
type: "cdp",
109+
...key,
110+
message: encodeJson({
111+
id: 1,
112+
method: "Browser.setDownloadBehavior",
113+
params: { behavior: "allowAndName", downloadPath: "/srv/downloads" },
114+
}),
115+
}),
116+
);
117+
debuggee.emit("Browser.downloadWillBegin", { guid: "guid-1", suggestedFilename: "r.csv" });
118+
expect(host.placeDownload(debuggee.tab.webContents, item)).toBe(true);
119+
expect(paths).toEqual(["/srv/downloads/guid-1"]);
120+
}),
121+
);
122+
});

0 commit comments

Comments
 (0)