mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
4c645f1619
Follow-ups from a post-merge review of #605. Each one is a comment thread on that PR. No behaviour changes to what the guard does — the three emitted `failure_stage` strings and the `CHROMIUM_WINDOW_HIDDEN` code are byte-for-byte identical, so telemetry stays joinable. ## Changes **Derive `failure_stage` from `action`.** It was a second free-form `string` parameter sitting next to `action`, and all three call sites passed a strictly redundant pair — `("tap", "chromium_tap_window_hidden")` and so on. Two adjacent same-typed params meant transposing them or mistyping the stage compiled cleanly and read fine in review. That carries extra weight here because #605 deliberately froze `chromium_scroll_window_hidden` to keep pre- and post-migration rows joinable. `action` is now `"tap" | "drag" | "scroll"`, so a typo is a compile error, and the docblock spells out all three literals so they stay greppable in `src`. **Pin the keyboard exemption.** Key events skip hit-testing and stay fast on a hidden window, so `keyboard` is deliberately unguarded. That was correct but lived only in a comment: adding the guard to `keyboard` would have made it refuse input that demonstrably works, and the suite would have stayed green. Now covered by `keyboard-chromium-unguarded.test.ts`. **Pin the `button` exemption too.** The same file now asserts that `assertSupported` rejects a Chromium device for `button`. The claim that `button` can never reach the guard rested entirely on a capability literal no test read — adding `chromium: { app: true }` to it reddened nothing. **Correct the same comment's claim about `button` — three times.** The first pass replaced the wrong rationale (that `button` skips the guard because key events bypass hit-testing) with the real one: its capability omits chromium. The replacement was itself wrong about where Chromium's button surface lives. `sendButton` is reached only from `handleWsCommand`, i.e. the chromium-server **WebSocket** channel — the HTTP router exposes screenshot / clipboard / fps / navigate / reload / history / viewport / stream and no button at all. It also over-claimed: `sendButton` emulates `Back` alone as an Alt+Left chord and throws for every other button, so "chromium hardware buttons exist" contradicted `button/index.ts` ("Chromium apps have no hardware buttons"). The wording now agrees with both neighbours. A third pass dropped the last false note in it: the lockout test said the gate stops `button` "before it can reach the guard", but `button` has no chromium branch and never imports the guard — there is no guard on its path to reach. **Give the keyboard measurement its missing condition.** `Input.dispatchKeyEvent` at 1–14 ms against 5002–5005 ms per mouse move, and "focus emulation keeps input fast even for a genuinely minimized window", describe the same scenario with opposite outcomes — those numbers can only come from a window where focus emulation was not in effect. The docblock says so, and names focus emulation alone: `primePageSession` applies it to every session at connect, so the probe reads "hidden" only where the runtime refused it. The anti-throttling launch flags are not part of that condition and do not narrow the guard's reach. A window carrying all three, minimized without emulation, still reads "hidden" and still costs ~5 s per `Input.dispatchMouseEvent` — Electron 42.5.0 5001–5005 ms and Chrome 152 5002–5010 ms, 3 trials each — so an argent-spawned app is no less exposed than an externally launched one. `boot-electron.ts`'s `ANTI_THROTTLING_ARGS` docblock, where that claim originated, is corrected to match, along with its test. **Two mechanisms that were stated unconditionally.** `Page.bringToFront` un-minimizes on Chrome 152 (`windowState` minimized → normal, `visibilityState` hidden → visible, mouse dispatch 5002–5006 ms → 16–25 ms, 3/3) and not on Electron 42.5.0 (returns ok, stays `"hidden"`, stays 5001–5005 ms, 3/3), so `flow-run`'s comment calls the outcome runtime-dependent rather than denying it. And the capability gate that keeps `button` off a Chromium device is not universal: `assertSupported` runs at the HTTP edge and in run-sequence's pre-flight, but `Registry.invokeTool` skips it, so a flow step dies resolving `services()` with `SimulatorServer.factory does not support platform "chromium"` rather than an `UnsupportedOperationError`. **Replace the try-boundary rationale.** It said the property access had to stay inside the `try` because fakes "may lack `cdp`" — but for such an api `chromium.cdp` yields `undefined` without throwing; the TypeError comes from reading `.send`, which is inside the `try` wherever `chromium.cdp` is read. Verified: hoisting `const client = chromium.cdp;` above the `try` leaves every relevant test green. The placement would only matter if `chromium` itself were nullish, which no registry path produces. **Cover the no-`cdp` probe path directly.** The existing test claimed to cover "api objects with no `cdp` at all" but used a rejecting `cdp.send` — a different failure mode. The real shape was exercised only incidentally, by bare fakes in `chromium-drag.test.ts` and `tools/gesture-tap.test.ts`. **Cover the other half of "a failed or empty read proves nothing".** Only the throw paths were pinned. Flipping the comparison to `value !== "visible"` left the entire suite green, so a probe that resolves without a usable value — `Runtime.evaluate` against a torn-down document — would have started refusing tap/drag/scroll on a fully visible window. **Pin the action word in the user-facing message.** Hardcoding `Cannot tap:` for all three tools also stayed green, so a scroll refusal reading "Cannot tap" could have shipped unnoticed. **Update `flow-run`'s comment.** It still named gesture-scroll as the only tool that fails fast on a hidden window; tap and drag have since #605. It also over-promised "fail fast": on the flow path `frontChromiumPage` has just primed focus emulation, so the three tools proceed. They carry the guard; whether it fires depends on the mitigations. ## Verification - Full gate on the branch: `tsc --build`, `typecheck:tests`, `eslint --max-warnings 0`, `prettier --check`, and the whole tool-server suite sharded — **360 files / 4504 passed, 1 skipped, 0 failed**. - Every empirical claim in the comments was re-measured on a live CDP testbed rather than carried over: an Electron 42.5.0 app and Chrome 152, each launched with and without the three `ANTI_THROTTLING_ARGS` plus `--remote-debugging-port`, minimized, probed for `document.visibilityState` and per-event `Input.dispatchMouseEvent` / `Input.dispatchKeyEvent` latency before any focus emulation, then after `Page.bringToFront`, then after `Emulation.setFocusEmulationEnabled`. Runs were sequential, one browser at a time, 3 trials per condition. - The `button`-on-chromium flow path was run, not just read: `registry.invokeTool("button", { udid: "chromium-cdp-9222", button: "home" })` with the simulator-server blueprint registered throws `ToolExecutionError … SimulatorServer.factory does not support platform "chromium"`. - The measurements behind the keyboard exemption were taken on a real minimized Electron window (v42.5.0): `Input.dispatchKeyEvent` 1–14 ms, against 5002–5005 ms for each of ten consecutive `Input.dispatchMouseEvent` moves, with `Runtime.evaluate` staying at 1–3 ms. - Mutation-tested every pin rather than assuming it bites. Each of these fails the suite: adding the guard to `keyboard/chromium`; adding `chromium` to `button`'s capability; making the probe rethrow instead of swallowing; drifting the derived stage template; collapsing the stage to one action for all three tools; flipping the comparison to `value !== "visible"`; and hardcoding the action word in the message. - No docs update needed: comments only, no user-facing behaviour or tool-surface change. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Documentation** - Clarified how window minimization, focus emulation, and anti-throttling behavior affect Chromium interactions. - **Bug Fixes** - Standardized hidden-window handling and error reporting for tap, drag, and scroll gestures. - Preserved keyboard input on hidden Chromium windows. - **Tests** - Expanded coverage for hidden-window gestures, keyboard input, missing browser capabilities, and varied visibility responses. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
162 lines
6.5 KiB
TypeScript
162 lines
6.5 KiB
TypeScript
import { describe, it, expect, vi } from "vitest";
|
|
import { FAILURE_CODES, FailureError, getFailureSignal } from "@argent/registry";
|
|
import { gestureTapTool } from "../src/tools/gesture-tap";
|
|
import { gestureDragTool } from "../src/tools/gesture-drag";
|
|
import { gestureScrollTool } from "../src/tools/gesture-scroll";
|
|
|
|
// The hidden-window guard: a minimized / fully occluded Chromium window
|
|
// throttles compositor hit-testing, so every MOUSE dispatch stalls ~5s
|
|
// (measured live on an Electron testbed). tap, drag and scroll refuse up
|
|
// front with an actionable error; keyboard skips the guard because key events
|
|
// bypass hit-testing and stay fast on hidden windows, and button never reaches
|
|
// it — its capability omits chromium. In practice the guard is a backstop:
|
|
// focus emulation, applied to every session at connect, pins reported
|
|
// visibility to "visible", so the probe reads "hidden" only where the runtime
|
|
// refused it. The fakes below hard-code `visibility`, so what they pin is the
|
|
// refusal, not the reading that provokes it.
|
|
|
|
function fakeChromiumApi(visibility = "visible") {
|
|
return {
|
|
getViewport: () => ({ width: 800, height: 600, devicePixelRatio: 1 }),
|
|
dispatchMouseEvent: vi.fn().mockResolvedValue(undefined),
|
|
server: { sendWheel: vi.fn().mockResolvedValue(undefined) },
|
|
cdp: { send: vi.fn().mockResolvedValue({ result: { value: visibility } }) },
|
|
};
|
|
}
|
|
|
|
async function captureFailure(promise: Promise<unknown>): Promise<FailureError> {
|
|
try {
|
|
await promise;
|
|
} catch (err) {
|
|
expect(err).toBeInstanceOf(FailureError);
|
|
return err as FailureError;
|
|
}
|
|
throw new Error("expected the tool to refuse on a hidden window");
|
|
}
|
|
|
|
describe("hidden-window guard on chromium mouse tools", () => {
|
|
it("gesture-tap refuses before dispatching any mouse event", async () => {
|
|
const api = fakeChromiumApi("hidden");
|
|
const err = await captureFailure(
|
|
gestureTapTool.execute(
|
|
{ chromium: api } as never,
|
|
{
|
|
udid: "chromium-cdp-19222",
|
|
x: 0.5,
|
|
y: 0.5,
|
|
} as never
|
|
)
|
|
);
|
|
expect(err.message).toMatch(/^Cannot tap: the Chromium window is hidden/);
|
|
expect(getFailureSignal(err)?.error_code).toBe(FAILURE_CODES.CHROMIUM_WINDOW_HIDDEN);
|
|
expect(getFailureSignal(err)?.failure_stage).toBe("chromium_tap_window_hidden");
|
|
expect(api.dispatchMouseEvent).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it("gesture-drag refuses before pressing the mouse button", async () => {
|
|
const api = fakeChromiumApi("hidden");
|
|
const err = await captureFailure(
|
|
gestureDragTool.execute(
|
|
{ chromium: api } as never,
|
|
{
|
|
udid: "chromium-cdp-19222",
|
|
fromX: 0.2,
|
|
fromY: 0.5,
|
|
toX: 0.8,
|
|
toY: 0.5,
|
|
} as never
|
|
)
|
|
);
|
|
expect(err.message).toMatch(/^Cannot drag: the Chromium window is hidden/);
|
|
expect(getFailureSignal(err)?.error_code).toBe(FAILURE_CODES.CHROMIUM_WINDOW_HIDDEN);
|
|
expect(getFailureSignal(err)?.failure_stage).toBe("chromium_drag_window_hidden");
|
|
expect(api.dispatchMouseEvent).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it("gesture-scroll refuses before dispatching any wheel event", async () => {
|
|
const api = fakeChromiumApi("hidden");
|
|
const err = await captureFailure(
|
|
gestureScrollTool.execute(
|
|
{ chromium: api } as never,
|
|
{
|
|
udid: "chromium-cdp-19222",
|
|
x: 0.5,
|
|
y: 0.5,
|
|
deltaY: 0.25,
|
|
} as never
|
|
)
|
|
);
|
|
expect(err.message).toMatch(/^Cannot scroll: the Chromium window is hidden/);
|
|
expect(api.server.sendWheel).not.toHaveBeenCalled();
|
|
expect(getFailureSignal(err)?.error_code).toBe(FAILURE_CODES.CHROMIUM_WINDOW_HIDDEN);
|
|
// The scroll stage predates the CHROMIUM_WINDOW_HIDDEN migration, which kept
|
|
// the spelling so rows from either side of it stay joinable.
|
|
expect(getFailureSignal(err)?.failure_stage).toBe("chromium_scroll_window_hidden");
|
|
});
|
|
|
|
it("proceeds when the visibility probe rejects (mid-navigation teardown)", async () => {
|
|
// A failed probe proves nothing about visibility; only an explicit
|
|
// "hidden" refuses.
|
|
const api = fakeChromiumApi();
|
|
api.cdp.send = vi.fn().mockRejectedValue(new Error("Execution context was destroyed"));
|
|
const result = (await gestureTapTool.execute(
|
|
{ chromium: api } as never,
|
|
{
|
|
udid: "chromium-cdp-19222",
|
|
x: 0.5,
|
|
y: 0.5,
|
|
} as never
|
|
)) as { tapped: boolean };
|
|
expect(result.tapped).toBe(true);
|
|
expect(api.dispatchMouseEvent).toHaveBeenCalled();
|
|
});
|
|
|
|
it("proceeds when the api carries no `cdp` at all (bare tool fakes)", async () => {
|
|
// A distinct failure mode from the rejecting-send case above: here the
|
|
// throw is a TypeError raised while reading `.send` off `undefined`, not a
|
|
// rejected promise. The bare fakes in chromium-drag.test.ts and
|
|
// tools/gesture-tap.test.ts are exactly this shape, so the guard has to
|
|
// stay transparent to them; those files only cover it incidentally, for as
|
|
// long as neither of them grows a `cdp` key.
|
|
const api = {
|
|
getViewport: () => ({ width: 800, height: 600, devicePixelRatio: 1 }),
|
|
dispatchMouseEvent: vi.fn().mockResolvedValue(undefined),
|
|
};
|
|
const result = (await gestureTapTool.execute(
|
|
{ chromium: api } as never,
|
|
{
|
|
udid: "chromium-cdp-19222",
|
|
x: 0.5,
|
|
y: 0.5,
|
|
} as never
|
|
)) as { tapped: boolean };
|
|
expect(result.tapped).toBe(true);
|
|
expect(api.dispatchMouseEvent).toHaveBeenCalled();
|
|
});
|
|
|
|
// The other half of "only an explicit `hidden` refuses": a probe that resolves
|
|
// with anything else. `Runtime.evaluate` omits `value` when the expression
|
|
// throws an Error, and a page can redefine `visibilityState` to whatever it
|
|
// likes, so the literal "hidden" is the only thing that is evidence of a
|
|
// hidden window — refusing on anything else blocks a tap on a live one.
|
|
it.each([
|
|
["an empty result", {}],
|
|
["a result with no value", { result: {} }],
|
|
["a value of an unexpected shape", { result: { value: null } }],
|
|
["a string other than `hidden`", { result: { value: "prerender" } }],
|
|
])("proceeds when the probe resolves with %s", async (_label, resolved) => {
|
|
const api = fakeChromiumApi();
|
|
api.cdp.send = vi.fn().mockResolvedValue(resolved);
|
|
const result = (await gestureTapTool.execute(
|
|
{ chromium: api } as never,
|
|
{
|
|
udid: "chromium-cdp-19222",
|
|
x: 0.5,
|
|
y: 0.5,
|
|
} as never
|
|
)) as { tapped: boolean };
|
|
expect(result.tapped).toBe(true);
|
|
expect(api.dispatchMouseEvent).toHaveBeenCalled();
|
|
});
|
|
});
|