fix(web): load SQLite worker in VS Code Web (vscode.dev) (#418)#419
Conversation
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) <noreply@anthropic.com>
… CI guard 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) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 41 minutes and 54 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughVersion 1.3.6 fixes a hang in vscode.dev when loading gpkg files by switching the browser worker bundle from ESM to IIFE format and adapting the browser parentPort to handle worker messages correctly. The release includes comprehensive changelog entries, a version bump, and updated deployment configuration to publish only stable tags. Changesv1.3.6 Release - Browser Worker Compatibility Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the extension to version 1.3.6, resolving a critical issue where VS Code for Web databases hang on the loading screen. This is fixed by switching the browser worker bundle format to IIFE and introducing a Node-style parentPort adapter (createBrowserParentPort) for the browser runtime, supported by new unit and regression tests. A review comment identifies a bug in the createBrowserParentPort implementation where registering the same handler for multiple event types will overwrite its entry in the WeakMap, leading to unregistration failures and memory leaks. The reviewer provides a code suggestion to use a nested map structure to resolve this issue.
| const browserListeners = new WeakMap<EventListenerOrEventListenerObject, EventListener>(); | ||
|
|
||
| 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); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
The current implementation of createBrowserParentPort uses a single WeakMap keyed by the handler to store the wrapped listener. If the same handler is registered for multiple event types (e.g., both 'message' and 'error'), the second registration will overwrite the first one in the WeakMap.
This leads to two issues:
- Calling
offfor the first event type will retrieve the wrapped listener of the second event type and attempt to remove it, which is a no-op and fails to unregister the listener. - The original wrapped listener is leaked and can never be unregistered.
To fix this, we should store a nested Map of event names to wrapped listeners inside the WeakMap.
const browserListeners = new WeakMap<EventListenerOrEventListenerObject, Map<string, EventListener>>();
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);
let eventsMap = browserListeners.get(handler);
if (!eventsMap) {
eventsMap = new Map();
browserListeners.set(handler, eventsMap);
}
eventsMap.set(event, wrapped);
scope.addEventListener(event, wrapped);
},
off: (event: string, handler: EventListenerOrEventListenerObject) => {
const eventsMap = browserListeners.get(handler);
if (eventsMap) {
const wrapped = eventsMap.get(event);
if (wrapped) {
scope.removeEventListener(event, wrapped);
eventsMap.delete(event);
if (eventsMap.size === 0) {
browserListeners.delete(handler);
}
}
}
},
};![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d79cb470d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!existsSync(bundlePath)) { | ||
| assert.fail( | ||
| 'Missing out/worker-browser.js. Run `node scripts/build.mjs` before running this regression test.' |
There was a problem hiding this comment.
When npm test is run from a clean checkout, out/worker-browser.js will not exist because out/ is ignored and the test script does not build first; this new assertion makes the whole unit suite fail unless a previous node scripts/build.mjs happened to leave artifacts behind. Either have the test build to a temporary output or skip/structure this as a build-validation test so the documented standalone npm test workflow remains reliable.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/platform/threadPool.ts`:
- Around line 60-78: The current browserListeners WeakMap (used by on/off) is
keyed only by handler so registering the same handler for different events
overwrites earlier wrapped listeners and prevents proper removal; change
browserListeners to map by event then handler (for example: Map<string,
WeakMap<EventListenerOrEventListenerObject, EventListener>>) and update on() to
get/create the per-event WeakMap and store wrapped there, and update off() to
look up the per-event WeakMap and remove the specific wrapped listener before
calling scope.removeEventListener(event, wrapped); keep using the existing
symbols browserListeners, on, off, scope.addEventListener and
scope.removeEventListener.
In `@tests/unit/threadPool_browser_parentport.test.ts`:
- Around line 87-91: Add a new test that registers the same handler function
twice (e.g., reuse messageHandler) for two different events via parentPort.on
and then calls parentPort.off for each event, asserting that removedListeners
contains two entries with event names matching each call and listener equal to
the originally registered wrapped listener (use registeredListeners to compare).
This verifies that off() correctly removes the wrapped listener when the same
handler is reused (exercise the WeakMap keying behavior referenced in
threadPool.ts) by checking removedListeners.length increments and
removedListeners[n].listener === registeredListeners[n].listener for both
events.
- Around line 9-11: Add an import for the VS Code mock setup at the very top of
tests/unit/threadPool_browser_parentport.test.ts so mocks are initialized before
any other imports; specifically, insert "import
'tests/unit/vscode_mock_setup.ts';" as the first line above the existing imports
(which reference createBrowserParentPort) to ensure the VS Code mocks are loaded
prior to running the tests that import createBrowserParentPort.
In `@tests/unit/worker_browser_bundle.test.ts`:
- Around line 9-13: The test file is missing the VS Code mock initializer; add
the import './vscode_mock_setup'; as the very first import in
tests/unit/worker_browser_bundle.test.ts so mocks are registered before any
other imports (place it above the existing imports of describe/it, assert,
existsSync, readFileSync, path, vm) to ensure VS Code mocks are initialized
prior to module loading.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b8eb7fd4-0e4c-4260-846e-85186ba957f8
📒 Files selected for processing (8)
.github/workflows/release.yml.gitignoreCHANGELOG.mdpackage.jsonscripts/build.mjssrc/platform/threadPool.tstests/unit/threadPool_browser_parentport.test.tstests/unit/worker_browser_bundle.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build, typecheck & test
🧰 Additional context used
📓 Path-based instructions (3)
tests/unit/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
tests/unit/*.test.ts: UseObject.defineProperty(obj, prop, { value, writable: true, configurable: true })for setting readonly VS Code API fields likevscode.env.uiKindin unit tests
Importtests/unit/vscode_mock_setup.tsat the beginning of unit test files to ensure VS Code mocks are initialized before running tests
Files:
tests/unit/threadPool_browser_parentport.test.tstests/unit/worker_browser_bundle.test.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.ts: Use prepared statements with?placeholders for all query parameter values to prevent SQL injection
Always useescapeIdentifier()function for table and column names in SQL queries to prevent identifier-based SQL injection
UsevalidateSqlType()for all user-provided SQL types in DDL statements to prevent type-based SQL injection
Validate PRAGMA string values with the regex/^[a-zA-Z0-9_-]+$/whitelist and check numeric PRAGMA values withNumber.isFinite()
UseescapeLikePattern()for user input in LIKE queries with theESCAPE '\\'clause to prevent LIKE wildcard injection
Use zero-copy transfer for large binary data (ArrayBuffers) in RPC communication by wrapping with theTransferwrapper
UseSAVEPOINT/RELEASE/ROLLBACK TOinstead ofBEGIN TRANSACTIONinupdateCellBatchto safely handle nested transactions
Use thesafeRollback(context)helper when handling transaction errors to log failures instead of throwing, preventing secondary rollback errors
Check for SQLitejson_patch()availability at engine construction time and use it in UPDATE statements when available, falling back to JS-sideapplyMergePatch()when unavailable
UsegetNodeFs()fromsqlite-db.tsto safely require the Node.jsfsmodule, which returnsundefinedin browser environments
Checkimport.meta.env.VSCODE_BROWSER_EXTto conditionally handle environment-specific code paths for browser vs Node.js platforms
Use the Core RPC protocol defined insrc/core/rpc.tsfor all Worker communication and when the Extension invokes Webview methods
UsebuildMethodProxy()fromsrc/core/rpc.tsto create proxy objects that automatically serialize RPC calls to workers or the webview
Record database modifications inModificationTrackerviarecordModification()before committing changes to track undo/redo history
Write all executed SQL (both read and write operations) to the 'SQLite Explorer' output channel viaGlobalOutputChannel?.appendLine()for debugging...
Files:
src/platform/threadPool.ts
{src/**/*.ts,core/ui/modules/*.js}
📄 CodeRabbit inference engine (CLAUDE.md)
Serialize
Uint8Arrayusing the marker format{ __type: 'Uint8Array', base64: '...' }with exactly 2 keys to prevent collisions with user data
Files:
src/platform/threadPool.ts
🔇 Additional comments (7)
package.json (1)
6-6: LGTM!CHANGELOG.md (1)
3-52: LGTM!.github/workflows/release.yml (1)
16-19: LGTM!.gitignore (1)
36-40: LGTM!scripts/build.mjs (1)
181-186: LGTM!tests/unit/worker_browser_bundle.test.ts (1)
16-39: LGTM!src/platform/threadPool.ts (1)
92-104: LGTM!
…review) 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) <noreply@anthropic.com>
Review feedback addressed (pushed in 8d8a2e9)Thanks @coderabbitai, @gemini-code-assist, and Codex. Verdicts: ✅ Fixed —
|
|
Thank you for the update and for confirming the fix for the |
|
WeakMap fix + regression test — the per-handler inner
🐇 ✨ ✏️ Learnings added
✅ Actions performedComments resolved. Approval is disabled; enable |
…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) <noreply@anthropic.com>
Round 2 — Codex feedback addressed (pushed)Thanks @codex. ✅ Fixed —
|
|
To use Codex here, create an environment for this repo. |
1 participant
Summary
Fixes #418 — in VS Code for Web (vscode.dev / github.dev) every database file (
.db,.sqlite,.gpkg, …) hangs forever on the loading screen. Desktop VS Code and the website demo are unaffected because they use different worker paths.Root cause (reproduced live in a real browser, not theorized)
The browser SQLite worker (
out/worker-browser.js, spawned vianew Worker(blobUrl)inworkerFactory.ts) boots but immediately throws, so it never wires up its message handler — every host→worker RPC then times out silently and the viewer stays on "loading".Live console from the unfixed build:
src/databaseWorker.tscalls Node'sparentPort.on('message', …). In a browser Web Worker the global scope (DedicatedWorkerGlobalScope) exposesaddEventListener/postMessage, not Node's.on().threadPool.tshad setparentPort = globalThis, so.on(...)isundefined→TypeError→ handler never registered → RPC hangs.Fix
src/platform/threadPool.ts— provide a Node-styleparentPortadapter for the browser runtime via an exported, unit-testablecreateBrowserParentPort(scope)that maps.on('message', cb)→addEventListener('message', e => cb(e.data))(and passes other events, e.g.error, through).databaseWorker.tsis unchanged.scripts/build.mjs— emit the browser worker as IIFE instead of ESM. This is defensive hardening, not an independent cause of [Bug]: Cannot load gpkg in vscode.dev #418:databaseWorker.tshas no exports today, so the ESM output had no top-levelexport{}and still parsed as a classic worker. But loading an ESM bundle via classicnew Worker(blobUrl)is wrong-in-principle and would break the moment any top-level export is introduced. IIFE matches how the bundle is actually loaded.Tests (new regressions, both proven to fail when their fix is reverted)
tests/unit/worker_browser_bundle.test.ts— parses the builtout/worker-browser.jswithnode:vmas a classic script; fails if the bundle ever reverts to an ESM/module format.tests/unit/threadPool_browser_parentport.test.ts— covers the adapter: message-data delivery,errorpassthrough,postMessage/transfer forwarding, andoff().Verification
node scripts/build.mjs✓ ·npx tsc --noEmit -p tsconfig.json✓ ·npm test→ 332/332 passworkerFactory.tsdoes (classic blob-URL worker) → worker reachesReady for connections,initializeDatabase+runQuerysucceed (rows returned), zero worker errors. Reverting either fix reproduces the hang / fails the corresponding test.@vscode/test-webwith this build sideloaded (the same runtime vscode.dev serves) for manual click-through.Release prep (in the second commit)
Since this is the first change to cut a release after
v1.3.5, the1.3.6CHANGELOG section covers all 46 commits merged since thev1.3.5tag (this fix + perf, refactors, tests, CI, website, deps),package.jsonis bumped to1.3.6, andrelease.ymlnow skips hyphenated tags so a GitHub pre-release (v1.3.6-rc.1) won't trigger the stable-release job.Closes #418.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Chores