From 9ce4ca4b1ea424497e65d3f4e702800b8467b804 Mon Sep 17 00:00:00 2001 From: zknpr Date: Sun, 31 May 2026 21:37:40 +0200 Subject: [PATCH 1/4] fix(web): load SQLite worker in VS Code Web (vscode.dev) (#418) Every database hung on the loading screen in vscode.dev (works in the web demo and on desktop). The browser worker never booted, due to two stacked bugs, both confirmed live in a real browser: 1. out/worker-browser.js was bundled as ESM (format: 'esm'), but workerFactory.ts loads it as a CLASSIC worker via `new Worker(blobUrl)`. An ESM bundle emits a top-level `export{...}`, which is a SyntaxError in a classic worker ("Unexpected token 'export'"); the worker never started. 2. threadPool.ts set the browser worker's `parentPort = globalThis`, but databaseWorker.ts calls `parentPort.on('message', ...)`. The worker global scope exposes addEventListener, not Node's `.on()`, so even once the worker loaded it threw `TypeError: parentPort.on is not a function` and never wired up its message handler -> host RPC hangs forever. Fix: - Build the browser worker as IIFE (classic-worker compatible; matches extension-browser.js and the emscripten/sql.js classic-worker env). - Add a Node-style {on, off, postMessage} adapter over the worker global scope in threadPool.ts so databaseWorker.ts works unchanged. Verified end-to-end: a classic blob-URL worker built from this config loads, initializes sql.js, and runs queries. tsc clean, build green, 330 unit tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- scripts/build.mjs | 7 ++++++- src/platform/threadPool.ts | 31 ++++++++++++++++++++++++++++++- 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/scripts/build.mjs b/scripts/build.mjs index 1ff1804..d74b613 100644 --- a/scripts/build.mjs +++ b/scripts/build.mjs @@ -178,7 +178,12 @@ const compileBrowserWorker = () => ...baseWorkerConfig, outfile: resolve(outDir, 'worker-browser.js'), platform: 'browser', - format: 'esm', + // IIFE (not ESM): this bundle is loaded as a CLASSIC Web Worker via + // `new Worker(blobUrl)` in workerFactory.ts. An ESM bundle emits a top-level + // `export{...}` which a classic worker cannot parse ("SyntaxError: Unexpected + // token 'export'"), so the worker never boots and VS Code Web hangs on load. + // IIFE also keeps the classic-worker environment sql.js/emscripten expects. + format: 'iife', mainFields: ['browser', 'module', 'main'], external: ['fs/promises', 'path'], define: { diff --git a/src/platform/threadPool.ts b/src/platform/threadPool.ts index 112d3dd..bb761e2 100644 --- a/src/platform/threadPool.ts +++ b/src/platform/threadPool.ts @@ -54,7 +54,36 @@ if (isBrowserRuntime) { MessageChannelImpl = globalThis.MessageChannel; MessagePortImpl = globalThis.MessagePort; BroadcastChannelImpl = globalThis.BroadcastChannel; - parentPortImpl = globalThis; + // In a browser Web Worker the global scope (DedicatedWorkerGlobalScope) is the + // channel to the host, but it exposes addEventListener/postMessage — NOT the + // Node worker_threads `.on()` API that the worker entry (databaseWorker.ts) is + // written against. Without this adapter, `parentPort.on('message', ...)` throws + // `TypeError: parentPort.on is not a function`, so the worker never wires up its + // message handler and the host's RPC hangs forever (VS Code Web "stuck loading"). + const workerScope: { + postMessage(data: unknown, transfer?: Transferable[]): void; + addEventListener: EventTarget['addEventListener']; + removeEventListener: EventTarget['removeEventListener']; + } = globalThis as unknown as typeof workerScope; + const browserListeners = new WeakMap(); + parentPortImpl = { + postMessage: (data: unknown, transfer?: Transferable[]) => + transfer ? workerScope.postMessage(data, transfer) : workerScope.postMessage(data), + on: (event: string, handler: EventListenerOrEventListenerObject) => { + // Node's parentPort.on('message', cb) passes the message DATA directly; the + // DOM 'message' event wraps it in event.data. For other events (e.g. + // 'error') pass the event through so consumers can read `.message`. + const cb = handler as (payload: unknown) => void; + const wrapped: EventListener = (e: Event) => + cb(event === 'message' ? (e as MessageEvent).data : e); + browserListeners.set(handler, wrapped); + workerScope.addEventListener(event, wrapped); + }, + off: (event: string, handler: EventListenerOrEventListenerObject) => { + const wrapped = browserListeners.get(handler); + if (wrapped) workerScope.removeEventListener(event, wrapped); + }, + }; } else { // Node.js environment try { From d79cb470d1b1db3f19e3074c472d05ce5e08470a Mon Sep 17 00:00:00 2001 From: zknpr Date: Sun, 31 May 2026 22:46:13 +0200 Subject: [PATCH 2/4] =?UTF-8?q?chore(release):=20prep=201.3.6=20=E2=80=94?= =?UTF-8?q?=20regression=20tests,=20changelog,=20pre-release=20CI=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Builds on the #418 fix (browser worker parentPort adapter) committed earlier on this branch. - threadPool.ts: extract the browser parentPort adapter into an exported, unit-testable createBrowserParentPort() factory (behavior unchanged). - tests: add two regressions for #418 — - worker_browser_bundle.test.ts: parse out/worker-browser.js with node:vm as a classic script; fails if the bundle ever reverts to an ESM/module format (the classic-vs-module mismatch hardened in build.mjs). - threadPool_browser_parentport.test.ts: cover the adapter (message data delivery, error passthrough, postMessage forwarding, off()). - CHANGELOG.md: add the 1.3.6 section covering all changes since v1.3.5 (46 commits: #418 fix, perf, refactors, tests, CI, website, deps). - package.json: 1.3.5 -> 1.3.6. - release.yml: skip hyphenated tags (e.g. v1.3.6-rc.1) so GitHub pre-releases don't trigger the stable-release job. - .gitignore: ignore local browser/extension test-harness artifacts. Verified: node scripts/build.mjs OK; tsc --noEmit -p tsconfig.json clean; npm test 332/332 pass; browser e2e repro of the fixed worker passes (worker boots, initializeDatabase + runQuery succeed). Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/release.yml | 4 + .gitignore | 6 ++ CHANGELOG.md | 50 ++++++++++ package.json | 2 +- src/platform/threadPool.ts | 72 ++++++++------ .../threadPool_browser_parentport.test.ts | 93 +++++++++++++++++++ tests/unit/worker_browser_bundle.test.ts | 40 ++++++++ 7 files changed, 239 insertions(+), 28 deletions(-) create mode 100644 tests/unit/threadPool_browser_parentport.test.ts create mode 100644 tests/unit/worker_browser_bundle.test.ts diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 6d0004d..1598d55 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -13,6 +13,10 @@ permissions: jobs: release: runs-on: ubuntu-latest + # Only run for stable version tags (vX.Y.Z). Pre-release tags such as + # v1.3.6-rc.1 contain a hyphen and are skipped here — they are published + # as GitHub pre-releases manually with the built .vsix attached. + if: ${{ !contains(github.ref_name, '-') }} steps: - name: Checkout code uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 diff --git a/.gitignore b/.gitignore index 200e073..f7fec62 100644 --- a/.gitignore +++ b/.gitignore @@ -32,3 +32,9 @@ Thumbs.db *.zip tmp/ .worktrees/ + +# Local browser/extension test harness (not part of the repo) +.playwright-mcp/ +.vscode-test-web/ +_repro_*.html +vscode-web-test-db.png diff --git a/CHANGELOG.md b/CHANGELOG.md index 1711693..2532984 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,55 @@ # Changelog +## 1.3.6 + +### Bug Fixes + +- **VS Code for Web (vscode.dev): databases stuck on the loading screen**: Fixed the browser SQLite worker never starting. The worker entry point calls Node's `parentPort.on('message', …)`, but inside a browser Web Worker the global scope only exposes `addEventListener`, not Node's `.on()` — so the worker threw `TypeError: parentPort.on is not a function` before wiring up its message handler, and every database operation silently timed out. A Node-style `parentPort` adapter is now provided for the browser runtime. This affected all database files (`.db`, `.sqlite`, `.gpkg`, …) in the web build; the desktop build and the website demo were unaffected because they use different worker paths (#418). +- **Error Cause Preservation**: The worker now preserves the original error `cause` when re-throwing a database open failure, so the underlying reason is no longer lost (#351). +- **Web Demo Query Parameterization**: Parameterized the filter queries in the web-demo worker, removing string-interpolated SQL in the demo data path (#401). + +### Performance + +- **Batched ALTER TABLE ADD COLUMN**: `undoColumnDrop` batches its `ADD COLUMN` statements into a single call instead of one per column (#405). +- **Batched ALTER TABLE DROP COLUMN**: `deleteColumns` batches its `DROP COLUMN` statements into a single call (#408). +- **Cached JSON Patches**: The `updateCells` JS fallback caches parsed JSON patches across a batch instead of re-parsing per row (#387). +- **updateCellBatch SAVEPOINT Fallback**: The sequential `hostBridge` cell-batch fallback is wrapped in a single `SAVEPOINT` to avoid per-row transaction overhead (#364). +- **Grid Selection Allocation**: Avoided string-key allocations in batch cell/column selection for large selections (#375). +- **Allocation Trimming**: Dropped `Object.keys` allocations in the JSON-merge-patch key iteration (#362) and in serialization, and tidied the `Uint8Array` marker check (#356). + +### Improvements + +- **Browser Worker Bundle Format**: The browser worker bundle (`out/worker-browser.js`) is now emitted as a classic-worker (IIFE) bundle to match how it is loaded (`new Worker(blobUrl)`), removing a latent module-vs-classic mismatch. Added regression tests that fail if the bundle reverts to ESM or if the browser `parentPort` adapter is removed (#418). +- **UI Module Extraction**: Extracted the pure batch-update logic out of `sidebar.js` into a DOM-free, unit-testable module (#416); split `renderSidebar` into helper functions; and modularized the grid render/events and viewer initialization paths. +- **Code Organization**: Extracted file-signature byte arrays into a shared `FILE_SIGNATURES` table (#374), per-format streaming into `getFormatHelper()` (#380), `maskSensitiveData()` into helpers (#398), a `requireEngine()` helper in `createWorkerEndpoint` (#357), and a `DatabaseMethodName` type alias (#386); converted panel-handler arrow-fields to methods (#383). +- **Type Safety**: Tightened the webview message-handler types (`any` → `unknown`) (#377). +- **Webview Hygiene**: Dropped a CSP-blocked inline `onerror` handler from the codicons `` (#353), and removed several unused imports across `edit.js` (#359), `dnd.js` (#354), `blob-inspector.js` (#355), and `main.ts` (#352). + +### Tests + +- Expanded unit coverage: activation/deactivation entrypoint (#406); `disposeAll` null-skip and child `AggregateError` (#403); WASM `establishConnection` failure + worker termination (#402); RPC transfer-fallback warning args (#394); `doTry` null/non-error branches (#391); `deleteColumns` undo-history fetch-error path (#385); delegated worker-endpoint operations after init (#370); comprehensive virtual-filesystem `writeFile` + delete/rename/stat/watch coverage (#365); and consolidated overlapping error-path coverage. + +### CI / Tooling + +- **PR Workflow**: Added a pull-request workflow that builds, typechecks, and tests on every PR, and fixed the latent type errors it surfaced (#414); addressed follow-up review feedback (#417). + +### Documentation + +- Added GitHub community health files (#343); clarified the auto-save failure comment (#358) and rephrased the save-edit comment in `edit.js` (#382). + +### Website + +- Migrated the website to Tailwind CSS v4 (#415). +- Bumped `react`/`react-dom` to 19.2.6 (#344, #347), `@types/node` to 25.9.1 (#348), and `eslint` to 10.4.0 (#349). + +### Dependencies + +**Extension:** + +- @types/node 25.9.0 → 25.9.1 (#345) +- tsx 4.21.0 → 4.22.3 (#346) +- tmp 0.2.5 → 0.2.7 (transitive, npm_and_yarn group) (#381) + ## 1.3.5 ### Security diff --git a/package.json b/package.json index 30f9ecc..1eec4f0 100644 --- a/package.json +++ b/package.json @@ -3,7 +3,7 @@ "name": "sqlite-explorer", "displayName": "SQLite Explorer", "description": "A powerful SQLite database viewer and editor for VS Code", - "version": "1.3.5", + "version": "1.3.6", "publisher": "zknpr", "license": "MIT", "repository": { diff --git a/src/platform/threadPool.ts b/src/platform/threadPool.ts index bb761e2..d3461e8 100644 --- a/src/platform/threadPool.ts +++ b/src/platform/threadPool.ts @@ -37,19 +37,59 @@ interface NodeMessagePort { off(event: string, handler: EventListenerOrEventListenerObject, options?: object): void; } +/** + * Minimal browser worker global scope surface used by the parentPort adapter. + * DedicatedWorkerGlobalScope provides these DOM APIs for communication with the + * spawning context. + */ +interface BrowserParentPortScope { + postMessage(data: unknown, transfer?: Transferable[]): void; + addEventListener: EventTarget['addEventListener']; + removeEventListener: EventTarget['removeEventListener']; +} + +/** + * Create a Node-style parentPort facade for browser workers. + * + * Node's parentPort.on('message', cb) delivers the message payload directly, + * while browser worker "message" listeners receive a MessageEvent whose data + * property contains the payload. The adapter preserves Node-style delivery for + * message events and forwards other events, such as "error", unchanged. + */ +export function createBrowserParentPort(scope: BrowserParentPortScope): NodeMessagePort { + const browserListeners = new WeakMap(); + + return { + postMessage: (data: unknown, transfer?: Transferable[]) => + transfer ? scope.postMessage(data, transfer) : scope.postMessage(data), + on: (event: string, handler: EventListenerOrEventListenerObject) => { + // Node's parentPort.on('message', cb) passes the message DATA directly; the + // DOM 'message' event wraps it in event.data. For other events (e.g. + // 'error') pass the event through so consumers can read `.message`. + const cb = handler as (payload: unknown) => void; + const wrapped: EventListener = (e: Event) => + cb(event === 'message' ? (e as MessageEvent).data : e); + browserListeners.set(handler, wrapped); + scope.addEventListener(event, wrapped); + }, + off: (event: string, handler: EventListenerOrEventListenerObject) => { + const wrapped = browserListeners.get(handler); + if (wrapped) scope.removeEventListener(event, wrapped); + }, + }; +} + // ============================================================================ // Runtime Detection and API Export // ============================================================================ -const isBrowserRuntime = import.meta.env?.VSCODE_BROWSER_EXT; - let WorkerImpl: any; let MessageChannelImpl: any; let MessagePortImpl: any; let BroadcastChannelImpl: any; let parentPortImpl: any; -if (isBrowserRuntime) { +if (import.meta.env?.VSCODE_BROWSER_EXT) { WorkerImpl = globalThis.Worker; MessageChannelImpl = globalThis.MessageChannel; MessagePortImpl = globalThis.MessagePort; @@ -60,30 +100,8 @@ if (isBrowserRuntime) { // written against. Without this adapter, `parentPort.on('message', ...)` throws // `TypeError: parentPort.on is not a function`, so the worker never wires up its // message handler and the host's RPC hangs forever (VS Code Web "stuck loading"). - const workerScope: { - postMessage(data: unknown, transfer?: Transferable[]): void; - addEventListener: EventTarget['addEventListener']; - removeEventListener: EventTarget['removeEventListener']; - } = globalThis as unknown as typeof workerScope; - const browserListeners = new WeakMap(); - parentPortImpl = { - postMessage: (data: unknown, transfer?: Transferable[]) => - transfer ? workerScope.postMessage(data, transfer) : workerScope.postMessage(data), - on: (event: string, handler: EventListenerOrEventListenerObject) => { - // Node's parentPort.on('message', cb) passes the message DATA directly; the - // DOM 'message' event wraps it in event.data. For other events (e.g. - // 'error') pass the event through so consumers can read `.message`. - const cb = handler as (payload: unknown) => void; - const wrapped: EventListener = (e: Event) => - cb(event === 'message' ? (e as MessageEvent).data : e); - browserListeners.set(handler, wrapped); - workerScope.addEventListener(event, wrapped); - }, - off: (event: string, handler: EventListenerOrEventListenerObject) => { - const wrapped = browserListeners.get(handler); - if (wrapped) workerScope.removeEventListener(event, wrapped); - }, - }; + const workerScope = globalThis as unknown as BrowserParentPortScope; + parentPortImpl = createBrowserParentPort(workerScope); } else { // Node.js environment try { diff --git a/tests/unit/threadPool_browser_parentport.test.ts b/tests/unit/threadPool_browser_parentport.test.ts new file mode 100644 index 0000000..c0ee35c --- /dev/null +++ b/tests/unit/threadPool_browser_parentport.test.ts @@ -0,0 +1,93 @@ +/** + * Regression tests for the browser parentPort adapter. + * + * databaseWorker.ts expects Node's worker_threads parentPort shape, while a + * DedicatedWorkerGlobalScope exposes DOM event APIs. These tests exercise the + * adapter against a fake worker global scope so the browser behavior is covered + * without depending on a real browser. + */ +import { describe, it } from 'node:test'; +import assert from 'node:assert'; +import { createBrowserParentPort } from '../../src/platform/threadPool'; + +describe('createBrowserParentPort', () => { + it('adapts browser worker scope events to Node-style parentPort methods', () => { + const registeredListeners: Array<{ + event: string; + listener: EventListenerOrEventListenerObject; + }> = []; + const removedListeners: Array<{ + event: string; + listener: EventListenerOrEventListenerObject; + }> = []; + const postedMessages: Array<{ data: unknown; transfer?: Transferable[] }> = []; + + const fakeScope = { + postMessage(data: unknown, transfer?: Transferable[]) { + // The fake scope records the exact payload and transfer list forwarded by + // the adapter so the test can verify postMessage does not transform them. + postedMessages.push({ data, transfer }); + }, + addEventListener(event: string, listener: EventListenerOrEventListenerObject) { + // The fake scope stores listeners exactly as registered, allowing the + // test to invoke the wrapped DOM listener with controlled event objects. + registeredListeners.push({ event, listener }); + }, + removeEventListener(event: string, listener: EventListenerOrEventListenerObject) { + // The fake scope records removals so off() can be checked against the + // wrapped listener that on() installed. + removedListeners.push({ event, listener }); + }, + }; + + const parentPort = createBrowserParentPort(fakeScope); + + assert.strictEqual(typeof parentPort.postMessage, 'function'); + assert.strictEqual(typeof parentPort.on, 'function'); + assert.strictEqual(typeof parentPort.off, 'function'); + + const receivedMessages: unknown[] = []; + const messageHandler = (payload: unknown) => { + receivedMessages.push(payload); + }; + + parentPort.on('message', messageHandler); + + assert.strictEqual(registeredListeners.length, 1); + assert.strictEqual(registeredListeners[0].event, 'message'); + + const messageListener = registeredListeners[0].listener as EventListener; + messageListener({ data: 'database-bytes' } as MessageEvent); + + assert.deepStrictEqual(receivedMessages, ['database-bytes']); + + const receivedErrors: unknown[] = []; + const errorHandler = (event: unknown) => { + receivedErrors.push(event); + }; + const errorEvent = new Event('error'); + + parentPort.on('error', errorHandler); + + assert.strictEqual(registeredListeners.length, 2); + assert.strictEqual(registeredListeners[1].event, 'error'); + + const errorListener = registeredListeners[1].listener as EventListener; + errorListener(errorEvent); + + assert.deepStrictEqual(receivedErrors, [errorEvent]); + + const transferBuffer = new ArrayBuffer(8); + parentPort.postMessage({ ready: true }, [transferBuffer]); + + assert.deepStrictEqual(postedMessages, [ + { data: { ready: true }, transfer: [transferBuffer] }, + ]); + + parentPort.off('message', messageHandler); + + assert.strictEqual(removedListeners.length, 1); + assert.strictEqual(removedListeners[0].event, 'message'); + assert.strictEqual(removedListeners[0].listener, registeredListeners[0].listener); + }); +}); diff --git a/tests/unit/worker_browser_bundle.test.ts b/tests/unit/worker_browser_bundle.test.ts new file mode 100644 index 0000000..eb5c4d6 --- /dev/null +++ b/tests/unit/worker_browser_bundle.test.ts @@ -0,0 +1,40 @@ +/** + * Regression tests for the browser worker bundle format. + * + * VS Code Web loads out/worker-browser.js as a classic Web Worker from a blob + * URL. Classic workers parse scripts with the normal script grammar, so this + * test compiles the bundle with node:vm to catch module-only syntax such as a + * top-level export before it can ship. + */ +import { describe, it } from 'node:test'; +import assert from 'node:assert'; +import { existsSync, readFileSync } from 'node:fs'; +import path from 'node:path'; +import vm from 'node:vm'; + +describe('browser worker bundle', () => { + it('parses as a classic worker script', () => { + const bundlePath = path.resolve(process.cwd(), 'out/worker-browser.js'); + + if (!existsSync(bundlePath)) { + assert.fail( + 'Missing out/worker-browser.js. Run `node scripts/build.mjs` before running this regression test.' + ); + } + + const source = readFileSync(bundlePath, 'utf8'); + + const isIifeBundle = /^(?:"use strict";)?\s*\(\s*(?:\(\)\s*=>|function\s*\()/.test( + source.trimStart() + ); + assert.ok( + isIifeBundle, + 'out/worker-browser.js must be emitted as an IIFE classic-worker bundle' + ); + + assert.doesNotThrow( + () => new vm.Script(source, { filename: bundlePath }), + 'out/worker-browser.js must parse as a classic worker script' + ); + }); +}); From af0f2d4f060b615f78bbc22eefcb688832b0d059 Mon Sep 17 00:00:00 2001 From: zknpr Date: Sun, 31 May 2026 22:57:28 +0200 Subject: [PATCH 3/4] fix(web): key browser parentPort listeners by (handler, event) (#419 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the WeakMap reuse bug flagged independently by Gemini and CodeRabbit on PR #419. createBrowserParentPort keyed wrapped listeners by handler alone, so registering the same handler for two events (e.g. one callback for both 'message' and 'error') would overwrite the first wrapper — leaking that DOM listener and making off() remove the wrong one. Key by (handler, event) via a per-handler inner Map; off() now removes the exact wrapper for that event and prunes empty maps. Not triggered by the current worker (databaseWorker.ts uses distinct handlers and never calls off()), but createBrowserParentPort is exported and unit-tested as a reusable primitive, so the latent bug is worth fixing. Added a regression test that registers one handler for two events and asserts off() removes the correct per-event wrapper — verified to FAIL against the old flat-key adapter and pass with this fix. Verified: tsc --noEmit clean; npm test 333/333 pass; browser e2e of the rebuilt worker still boots and runs queries. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/platform/threadPool.ts | 23 ++++++++-- .../threadPool_browser_parentport.test.ts | 46 +++++++++++++++++++ 2 files changed, 65 insertions(+), 4 deletions(-) diff --git a/src/platform/threadPool.ts b/src/platform/threadPool.ts index d3461e8..0d23683 100644 --- a/src/platform/threadPool.ts +++ b/src/platform/threadPool.ts @@ -57,7 +57,12 @@ interface BrowserParentPortScope { * message events and forwards other events, such as "error", unchanged. */ export function createBrowserParentPort(scope: BrowserParentPortScope): NodeMessagePort { - const browserListeners = new WeakMap(); + // Keyed by handler, then by event name. A single handler may be registered for + // more than one event (e.g. the same callback for 'message' and 'error'), so a + // flat handler->wrapped map would let the second registration overwrite the + // first — leaking the earlier DOM listener and making off() remove the wrong + // one. The per-event inner map keeps each (handler, event) wrapper distinct. + const browserListeners = new WeakMap>(); return { postMessage: (data: unknown, transfer?: Transferable[]) => @@ -69,12 +74,22 @@ export function createBrowserParentPort(scope: BrowserParentPortScope): NodeMess const cb = handler as (payload: unknown) => void; const wrapped: EventListener = (e: Event) => cb(event === 'message' ? (e as MessageEvent).data : e); - browserListeners.set(handler, wrapped); + let byEvent = browserListeners.get(handler); + if (!byEvent) { + byEvent = new Map(); + browserListeners.set(handler, byEvent); + } + byEvent.set(event, wrapped); scope.addEventListener(event, wrapped); }, off: (event: string, handler: EventListenerOrEventListenerObject) => { - const wrapped = browserListeners.get(handler); - if (wrapped) scope.removeEventListener(event, wrapped); + const byEvent = browserListeners.get(handler); + const wrapped = byEvent?.get(event); + if (wrapped) { + scope.removeEventListener(event, wrapped); + byEvent!.delete(event); + if (byEvent!.size === 0) browserListeners.delete(handler); + } }, }; } diff --git a/tests/unit/threadPool_browser_parentport.test.ts b/tests/unit/threadPool_browser_parentport.test.ts index c0ee35c..34c4af6 100644 --- a/tests/unit/threadPool_browser_parentport.test.ts +++ b/tests/unit/threadPool_browser_parentport.test.ts @@ -90,4 +90,50 @@ describe('createBrowserParentPort', () => { assert.strictEqual(removedListeners[0].event, 'message'); assert.strictEqual(removedListeners[0].listener, registeredListeners[0].listener); }); + + it('removes the correct wrapped listener when one handler is reused for two events', () => { + // Regression guard: the adapter must key wrapped listeners by (handler, event), + // not by handler alone. With a flat handler->wrapped map, registering the same + // handler for 'message' then 'error' would overwrite the first wrapper, so + // off('message', …) would remove the wrong (error) listener and leak the + // message one. This reuses a single handler across both events and asserts + // each off() removes the exact wrapper that on() installed for that event. + const registeredListeners: Array<{ event: string; listener: EventListener }> = []; + const removedListeners: Array<{ event: string; listener: EventListener }> = []; + + const fakeScope = { + postMessage() {}, + addEventListener(event: string, listener: EventListenerOrEventListenerObject) { + registeredListeners.push({ event, listener: listener as EventListener }); + }, + removeEventListener(event: string, listener: EventListenerOrEventListenerObject) { + removedListeners.push({ event, listener: listener as EventListener }); + }, + }; + + const parentPort = createBrowserParentPort(fakeScope); + + // Same handler reference registered for two distinct events. + const sharedHandler = () => {}; + parentPort.on('message', sharedHandler); + parentPort.on('error', sharedHandler); + + assert.strictEqual(registeredListeners.length, 2); + const messageWrapped = registeredListeners[0].listener; + const errorWrapped = registeredListeners[1].listener; + // Each event must get its OWN wrapper (the bug would reuse/overwrite one). + assert.notStrictEqual(messageWrapped, errorWrapped); + + parentPort.off('message', sharedHandler); + parentPort.off('error', sharedHandler); + + assert.strictEqual(removedListeners.length, 2); + assert.deepStrictEqual( + removedListeners.map(r => r.event), + ['message', 'error'] + ); + // The wrapper removed for each event matches the one registered for it. + assert.strictEqual(removedListeners[0].listener, messageWrapped); + assert.strictEqual(removedListeners[1].listener, errorWrapped); + }); }); From a4440ecd7e62eaaeffca67bcbf9513aee9e47178 Mon Sep 17 00:00:00 2001 From: zknpr Date: Sun, 31 May 2026 23:04:59 +0200 Subject: [PATCH 4/4] test(web): skip bundle test when out/ not built; drop dead types (#419 review) Addresses Codex's P2 on PR #419. - worker_browser_bundle.test.ts validates a build artifact (out/worker-browser.js), which is gitignored and not produced by `npm test`. It previously assert.fail()ed when the bundle was absent, breaking a clean `npm ci && npm test`. It now skips (with a hint) when the bundle is missing; CI builds before tests, so the real assertions still run there. - threadPool.ts: remove the now-unused BrowserMessagePort interface and the MessageReceiver alias it depended on (long-standing dead scaffolding surfaced after the createBrowserParentPort refactor started using NodeMessagePort). Verified: tsc --noEmit clean; `npm test` 334/334 with bundle built, and 1 skipped (0 failed) on a clean tree with no build; browser e2e of the rebuilt worker passes. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/platform/threadPool.ts | 14 -------------- tests/unit/worker_browser_bundle.test.ts | 11 +++++++---- 2 files changed, 7 insertions(+), 18 deletions(-) diff --git a/src/platform/threadPool.ts b/src/platform/threadPool.ts index 0d23683..94596a5 100644 --- a/src/platform/threadPool.ts +++ b/src/platform/threadPool.ts @@ -13,20 +13,6 @@ // Type Definitions for Cross-Platform Message Passing // ============================================================================ -/** - * Minimal event receiver interface for message handling. - * Matches the common subset of browser EventTarget and Node.js EventEmitter. - */ -type MessageReceiver = Pick; - -/** - * Browser-style message port interface. - * Used for communication in web worker environments. - */ -interface BrowserMessagePort extends MessageReceiver { - postMessage(data: unknown, transfer?: Transferable[]): void; -} - /** * Node.js-style message port interface. * Used for communication in Node.js worker_threads. diff --git a/tests/unit/worker_browser_bundle.test.ts b/tests/unit/worker_browser_bundle.test.ts index eb5c4d6..bcf77b0 100644 --- a/tests/unit/worker_browser_bundle.test.ts +++ b/tests/unit/worker_browser_bundle.test.ts @@ -13,13 +13,16 @@ import path from 'node:path'; import vm from 'node:vm'; describe('browser worker bundle', () => { - it('parses as a classic worker script', () => { + it('parses as a classic worker script', (t) => { const bundlePath = path.resolve(process.cwd(), 'out/worker-browser.js'); + // This validates a build artifact, not source. `out/` is gitignored and the + // `npm test` script does not build first, so on a clean checkout the bundle + // may be absent — skip (don't fail the suite) with a clear hint. CI runs + // `node scripts/build.mjs` before tests, so there the assertions always run. if (!existsSync(bundlePath)) { - assert.fail( - 'Missing out/worker-browser.js. Run `node scripts/build.mjs` before running this regression test.' - ); + t.skip('out/worker-browser.js not built — run `node scripts/build.mjs` first (CI builds before tests).'); + return; } const source = readFileSync(bundlePath, 'utf8');