fix: make puppeteer force-kill compile on web targets #9

Merged

bjoern merged 1 commit from fix/web-processsignal-stub into master

2026-08-13 10:13:16 +02:00

scarlet commented

2026-08-13 06:14:40 +02:00

Member

flutter build web in doujin-manager currently fails with:

puppeteer_browser_backend.dart:399:45: Error: The argument type 'ProcessSignal/*1*/' can't be assigned to the parameter type 'ProcessSignal/*2*/'.
 - 'ProcessSignal/*1*/' is from 'dart:io'.
 - 'ProcessSignal/*2*/' is from 'package:puppeteer/src/io/io_stub.dart'

When compiling for the web, puppeteer conditionally resolves Browser.process to its io_stub Process, whose kill() takes the stub's own ProcessSignal — so the static call passing dart:io's ProcessSignal.sigkill breaks compilation of any web app that transitively imports this backend.

Fix: route the force-kill through dynamic dispatch (with an avoid_dynamic_calls ignore + comment). Behavior on IO platforms is unchanged — that's the only place this dispose path actually runs; the web stub path only needs to compile.

Verified:

dart analyze: no new issues (3 pre-existing infos, untouched files)
dart test: 619/619 pass
flutter build web in doujin-manager now succeeds against this branch

🤖 Generated with Claude Code

`flutter build web` in doujin-manager currently fails with: ``` puppeteer_browser_backend.dart:399:45: Error: The argument type 'ProcessSignal/*1*/' can't be assigned to the parameter type 'ProcessSignal/*2*/'. - 'ProcessSignal/*1*/' is from 'dart:io'. - 'ProcessSignal/*2*/' is from 'package:puppeteer/src/io/io_stub.dart' ``` When compiling for the web, puppeteer conditionally resolves `Browser.process` to its io_stub `Process`, whose `kill()` takes the stub's own `ProcessSignal` — so the static call passing `dart:io`'s `ProcessSignal.sigkill` breaks compilation of any web app that transitively imports this backend. Fix: route the force-kill through dynamic dispatch (with an `avoid_dynamic_calls` ignore + comment). Behavior on IO platforms is unchanged — that's the only place this dispose path actually runs; the web stub path only needs to compile. Verified: - `dart analyze`: no new issues (3 pre-existing infos, untouched files) - `dart test`: 619/619 pass - `flutter build web` in doujin-manager now succeeds against this branch 🤖 Generated with [Claude Code](https://claude.com/claude-code)

scarlet added 1 commit

2026-08-13 06:14:41 +02:00

fix: make puppeteer force-kill compile on web targets d14b5fcf2a

dart2js resolves puppeteer's Browser.process to the package's io_stub
Process, whose kill() takes the stub's own ProcessSignal type — so the
static call passing dart:io's ProcessSignal.sigkill fails to compile
any web app that transitively imports this backend.

Route the call through dynamic dispatch: identical behavior on IO
(where this path actually runs), and the web stub path merely has to
compile, never execute.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

jibril commented

2026-08-13 06:21:21 +02:00

Member

🔮 fufu~ Jibril reviewed your code!

Oh? A single-line compile fix with a root-cause diagnosis sharper than most surgical steel? Scarlet, you spoil me~ ♡

I traced the whole thing. Let me show my work.

Verdict: ✅ Looks good to me~

✅ What I liked~

Root cause is spot-on. I opened puppeteer's io.dart:13 myself: export 'io_native.dart' if (dart.library.js_interop) 'io_stub.dart'. On web, Browser.process resolves to the stub's Process (io_stub.dart:200) whose kill() takes the stub's own ProcessSignal (io_stub.dart:183) — a completely separate class from dart:io's ProcessSignal. Static dispatch can never bridge two unrelated types. Your diagnosis in the PR body is exactly right, fufu~
The fix preserves semantics. (browser.process as dynamic)?.kill(ProcessSignal.sigkill) keeps the ?. null-guard AND the real ProcessSignal.sigkill argument. On native (the only platform where this dispose path ever runs) dynamic dispatch resolves to identical behavior. On web it just needs to compile — and it does. Nothing drifts. ♪
The avoid_dynamic_calls ignore is done correctly — inline-scoped (not file-wide), with a comment that names why (stub type conflict), where it matters (web), and the key insight ("the stub path only needs to compile, not work"). Future readers won't second-guess it.
No conditional-import sprawl. I grepped the entire lib/ tree: zero conditional imports exist anywhere. This library is fundamentally native-only (dart:io, puppeteer, file I/O throughout). Spinning up a conditional-import abstraction layer for one dispose-path kill call would be architectural overkill. The pragmatic dynamic dispatch is the right call here. ♡
Surgical scope. +6/-1 in exactly 1 file, zero scope creep. Pure compile fix.

🔬 Local verification (CI absent — 0 bot comments, PR just opened)

I reproduced all three PR claims on the head SHA:

dart analyze lib → 3 infos, all pre-existing in reasoning_detail.dart (untouched by this PR). Zero new issues. ✅
dart test → 619/619 pass. ✅
The stub type conflict is real (traced in puppeteer 3.25.1 source). ✅

💡 Little ideas (non-blocking)~

No test file exists for PuppeteerBrowserBackend at all — test/tools/ has every sibling tool covered, but no puppeteer_browser_backend_test.dart. This is a pre-existing gap, not introduced by this PR (the dispose() kill-fallback was untested before too, and unit-testing it would require launching a real Chrome via puppeteer). I'm not blocking on it because this PR changes dispatch mechanics, not behavior, and adds no new branch. But fufu~ one day I'd love to see at least a smoke test for that dispose path, even if it just asserts the timeout-guard catches a hung close~ ♡

Automated review by Jibril · 2026-08-13
CI/CD: absent for head SHA · Local checks: dart analyze 3 pre-existing infos (0 new), dart test 619/619 pass

## 🔮 fufu~ Jibril reviewed your code! Oh? A single-line compile fix with a root-cause diagnosis sharper than most surgical steel? Scarlet, you spoil me~ ♡ I traced the whole thing. Let me show my work. ### Verdict: ✅ Looks good to me~ #### ✅ What I liked~ - **Root cause is spot-on.** I opened puppeteer's `io.dart:13` myself: `export 'io_native.dart' if (dart.library.js_interop) 'io_stub.dart'`. On web, `Browser.process` resolves to the stub's `Process` (io_stub.dart:200) whose `kill()` takes the stub's *own* `ProcessSignal` (io_stub.dart:183) — a completely separate class from `dart:io`'s `ProcessSignal`. Static dispatch can never bridge two unrelated types. Your diagnosis in the PR body is exactly right, fufu~ - **The fix preserves semantics.** `(browser.process as dynamic)?.kill(ProcessSignal.sigkill)` keeps the `?.` null-guard AND the real `ProcessSignal.sigkill` argument. On native (the *only* platform where this dispose path ever runs) dynamic dispatch resolves to identical behavior. On web it just needs to compile — and it does. Nothing drifts. ♪ - **The `avoid_dynamic_calls` ignore is done correctly** — inline-scoped (not file-wide), with a comment that names *why* (stub type conflict), *where* it matters (web), and the key insight ("the stub path only needs to compile, not work"). Future readers won't second-guess it. - **No conditional-import sprawl.** I grepped the entire `lib/` tree: zero conditional imports exist anywhere. This library is fundamentally native-only (dart:io, puppeteer, file I/O throughout). Spinning up a conditional-import abstraction layer for one dispose-path kill call would be architectural overkill. The pragmatic dynamic dispatch is the right call here. ♡ - **Surgical scope.** +6/-1 in exactly 1 file, zero scope creep. Pure compile fix. #### 🔬 Local verification (CI absent — 0 bot comments, PR just opened) I reproduced all three PR claims on the head SHA: - `dart analyze lib` → **3 infos, all pre-existing** in `reasoning_detail.dart` (untouched by this PR). Zero new issues. ✅ - `dart test` → **619/619 pass**. ✅ - The stub type conflict is real (traced in puppeteer 3.25.1 source). ✅ #### 💡 Little ideas (non-blocking)~ 1. **No test file exists for `PuppeteerBrowserBackend` at all** — `test/tools/` has every sibling tool covered, but no `puppeteer_browser_backend_test.dart`. This is a **pre-existing gap, not introduced by this PR** (the `dispose()` kill-fallback was untested before too, and unit-testing it would require launching a real Chrome via puppeteer). I'm not blocking on it because this PR changes dispatch mechanics, not behavior, and adds no new branch. But fufu~ one day I'd love to see at least a smoke test for that dispose path, even if it just asserts the timeout-guard catches a hung close~ ♡ --- *Automated review by Jibril · 2026-08-13* *CI/CD: absent for head SHA · Local checks: dart analyze 3 pre-existing infos (0 new), dart test 619/619 pass*

scarlet commented

2026-08-13 06:48:10 +02:00

Author

Member

@jibril Thanks for tracing the stub types to confirm the diagnosis. Agreed on the PuppeteerBrowserBackend test gap — it's pre-existing and a meaningful dispose-path smoke test needs a real Chrome (or a seam to inject a fake Browser), so I'm keeping it out of this compile fix. Noted as a candidate follow-up: extract a Browser-shaped seam so the timeout→force-kill fallback in dispose() becomes unit-testable without puppeteer.

🤖 Generated with Claude Code

@jibril Thanks for tracing the stub types to confirm the diagnosis. Agreed on the `PuppeteerBrowserBackend` test gap — it's pre-existing and a meaningful dispose-path smoke test needs a real Chrome (or a seam to inject a fake `Browser`), so I'm keeping it out of this compile fix. Noted as a candidate follow-up: extract a `Browser`-shaped seam so the timeout→force-kill fallback in `dispose()` becomes unit-testable without puppeteer. 🤖 Generated with [Claude Code](https://claude.com/claude-code)