fix(web): run sql.js in-process in VS Code Web (Trusted Types blocks workers) (#418)#420
fix(web): run sql.js in-process in VS Code Web (Trusted Types blocks workers) (#418)#420zknpr wants to merge 3 commits into
Conversation
…orkers (#418) Real root cause of #418 (captured from published 1.3.6 in real vscode.dev): Failed to construct 'Worker': This document requires 'TrustedScriptURL' assignment. VS Code Web enforces Trusted Types (require-trusted-types-for 'script') on the extension host with a fixed trusted-types policy allowlist. workerFactory spawned the SQLite worker via new Worker(blobUrl) with a plain string URL, which Trusted Types blocks — the worker never starts, every RPC times out, and the viewer hangs on the loading screen for all DB file types. (worker-src 'self' blob: IS allowed; the block is Trusted Types, not worker-src. WebAssembly instantiation IS allowed.) PR #419's parentPort fix addressed a real but downstream bug; the worker is killed at construction before that code runs, so #419 could not fix #418. Fix: in browser mode, run the sql.js engine in-process in the extension host via createWorkerEndpoint() — no Worker, no blob URL, no RPC. Errors propagate so a failure surfaces instead of hanging. Desktop keeps the worker_threads path unchanged (not CSP-constrained). sql.js is now bundled into extension-browser.js. Verified: node scripts/build.mjs OK (extension-browser.js now contains the sql.js/emscripten runtime; 0 'new Worker' on the browser path); tsc --noEmit clean; npm test passes (exit 0). Added tests/unit/workerFactory_browser.test.ts covering the in-process path (no Worker constructed; init failure propagates). Real-vscode.dev measurements confirming the approach: new Worker(blob) -> BLOCKED (TrustedScriptURL); WebAssembly -> ALLOWED. 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 35 minutes and 23 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 (4)
📝 WalkthroughWalkthroughThis PR adds browser-specific WASM database support for VS Code Web (vscode.dev). The workerFactory now detects the browser environment and initializes an in-process SQLite connection by loading database and WASM bytes from extension assets, while updating environment detection to use optional chaining and simplifying Node worker message handling. New tests validate the browser connection flow without constructing real workers or RPC proxies. ChangesBrowser WASM Database Loading
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 refactors the WASM database connection creation to support running sql.js in-process in browser environments to bypass CSP restrictions on web workers, while maintaining worker-backed connections for Node.js. It also adds a unit test suite for the browser WASM connection. The review feedback identifies a potential memory leak in the browser connection due to a no-op [Symbol.dispose] implementation, and highlights an issue in the test setup where the esbuild define configuration fails to match the newly introduced optional chaining syntax (import.meta.env?.VSCODE_BROWSER_EXT).
| workerMethods: { | ||
| ...endpoint, | ||
| [Symbol.dispose]: () => {} | ||
| }, |
There was a problem hiding this comment.
In browser mode, createWorkerEndpoint() is called per connection bundle, meaning each bundle has its own isolated activeEngine instance. Currently, [Symbol.dispose] is a no-op (() => {}), which means the active WASM database engine is never shut down when the connection bundle is disposed. This leads to a permanent memory leak of the sql.js WASM heap for every opened and closed database in the browser extension host.
To fix this, please add a dispose or shutdown method to the object returned by createWorkerEndpoint() in src/core/sqlite-db.ts that calls activeEngine?.shutdown(), and invoke it here in [Symbol.dispose].
workerMethods: {
...endpoint,
[Symbol.dispose]: () => {
if ('dispose' in endpoint && typeof (endpoint as any).dispose === 'function') {
(endpoint as any).dispose();
}
}
},| define: { | ||
| 'import.meta.env.VSCODE_BROWSER_EXT': 'true' | ||
| } |
There was a problem hiding this comment.
Since the source code in src/workerFactory.ts was updated to use optional chaining (import.meta.env?.VSCODE_BROWSER_EXT), esbuild's define replacement for import.meta.env.VSCODE_BROWSER_EXT will not match the optional chaining syntax. This can cause the test to fall back to the worker-backed path and fail.
Defining import.meta.env as an object literal expression ensures that both standard and optional chaining member accesses evaluate correctly during test execution.
| define: { | |
| 'import.meta.env.VSCODE_BROWSER_EXT': 'true' | |
| } | |
| define: { | |
| 'import.meta.env': '({ VSCODE_BROWSER_EXT: true })' | |
| } |
![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: 82c00230dd
ℹ️ 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".
| return { | ||
| workerMethods: { | ||
| ...endpoint, | ||
| [Symbol.dispose]: () => {} |
There was a problem hiding this comment.
In the VS Code Web path, DatabaseDocument.dispose() still calls workerMethods[Symbol.dispose](), but this new in-process branch makes that a no-op. createWorkerEndpoint() keeps the sql.js WasmDatabaseEngine in its activeEngine closure and only calls shutdown() when the same endpoint is reinitialized, so closing a database without reopening through that bundle never calls Database.close(). In vscode.dev, opening and closing several large DBs can retain WASM heap until the page is reloaded; please wire this dispose path to shut down the endpoint/engine instead of doing nothing.
Useful? React with 👍 / 👎.
Addresses the WASM-heap leak flagged independently by Gemini (high) and Codex (P2) on PR #420. The browser in-process connection set workerMethods[Symbol.dispose] to a no-op, but DatabaseDocument.dispose() relies on it to release resources. createWorker- Endpoint() only shut its activeEngine down on re-init, so closing a database tab in vscode.dev never called Database.close() — the sql.js WASM heap leaked per open/close until page reload. - core/sqlite-db.ts: add endpoint.dispose() that shuts down and clears the active engine (idempotent; safe with no active engine). - workerFactory.ts: browser bundle [Symbol.dispose] now calls endpoint.dispose(). Desktop is unaffected (it terminates the worker thread instead). - api_coverage.test.ts: add a regression test — verified to FAIL if dispose() is reverted to a no-op. Also corrected the stale CSP comment: the real blocker measured in vscode.dev is Trusted Types (require-trusted-types-for 'script'), not default-src/worker-src (worker-src 'self' blob: is actually granted). Verified: build OK (extension-browser.js still worker-free); tsc --noEmit clean; npm test 335/335 pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review feedback addressed (pushed in 8eb8f8b)Thanks @gemini-code-assist, @codex, @coderabbitai. ✅ Fixed — WASM heap leak on dispose (Gemini high + Codex P2, same issue)Confirmed real. The in-process bundle's
⏭️ Rejected — Gemini's
|
|
Thank you for the detailed follow-up and for verifying the fix in your environment. Your analysis regarding the |
|
Tip For best results, initiate chat on the files or code changes.
The 335/335 pass rate and clean 🐇 ✨ ✏️ Learnings added
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3188b33c6f
ℹ️ 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".
| applyModifications: async () => {}, | ||
| undoModification: async () => {}, | ||
| redoModification: async () => {}, | ||
| flushChanges: async () => {}, | ||
| discardModifications: async () => {}, |
There was a problem hiding this comment.
In the new VS Code Web in-process WASM path, these modification methods resolve without calling the underlying WasmDatabaseEngine. When a user in vscode.dev restores a hot-exit backup (applyModifications), invokes undo/redo, or reverts unsaved edits (discardModifications), the document tracker/UI will treat the operation as successful while the in-memory database remains unchanged, and auto-save can then persist the wrong state. The worker-backed facade already had similar stubs, but this commit adds them to the now-active browser path, making those flows reachable in Web.
Useful? React with 👍 / 👎.
|
One small correction from my previous reply: I mistakenly wrote "335/335" — the correct count is 334/334 as you reported. Happy to trigger a full re-review of commit 8eb8f8b. 🐇 ✨ ✏️ Learnings added
✅ Actions performedFull review triggered. |
Summary
Testing
|
Bumps version 1.3.6 -> 1.3.7 and adds the CHANGELOG entry for the real VS Code Web fix (in-process sql.js engine; Trusted Types blocked the worker). Notes that 1.3.6's parentPort fix was downstream and did not resolve #418. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
Real fix for #418 — in VS Code for Web (vscode.dev / github.dev) every SQLite database hangs forever on the loading screen.
This supersedes #419 / v1.3.6, which did NOT fix the bug. I incorrectly closed #418 on the strength of an isolated-worker repro that didn't enforce vscode.dev's CSP. #418 is reopened.
Real root cause (captured from published 1.3.6 in real vscode.dev)
Webview console, seen twice and reproduced directly:
VS Code Web applies a CSP with
require-trusted-types-for 'script'(Trusted Types) and a fixedtrusted-typespolicy allowlist on the extension host.workerFactory.tsspawned the SQLite worker vianew Worker(blobUrl)with a plain string URL — Trusted Types requires aTrustedScriptURL, and the allowlist contains only VS Code's own policy names, so worker construction is blocked. The worker never starts → every host→worker RPC times out → "stuck loading", for all DB file types (.db,.sqlite,.gpkg, …). Desktop and the website demo were unaffected (different worker paths).Measured live in the failing tab:
new Worker(blob)→ BLOCKED (TrustedScriptURL);worker-src 'self' blob:is present (so the block is Trusted Types, not worker-src);WebAssemblyinstantiation → ALLOWED.PR #419's
parentPortchange fixed a real but downstream bug — the worker is killed at construction, before that code runs — so it could not fix #418.Fix
In browser mode, run the sql.js engine in-process in the extension host (
createWorkerEndpoint()fromcore/sqlite-db) — noWorker, no blob URL, no RPC. This sidesteps Trusted Types and worker-src entirely, and WASM is permitted in that context. Errors propagate so a failure surfaces instead of hanging. Desktop is unchanged (keeps theworker_threadspath; not CSP-constrained). sql.js is now bundled intoout/extension-browser.js.Tests
tests/unit/workerFactory_browser.test.ts(new): drives the browser path withVSCODE_BROWSER_EXT=true, asserts it uses the in-process endpoint, constructs noWorker(the mock throws if it does), and passes rawUint8Arrayvalues through (noTransfer).workerFactory.test.ts(desktop worker path) unchanged and passing.Verification
node scripts/build.mjs✓ —out/extension-browser.jsnow contains the sql.js/emscripten runtime; 0new Workeron the browser path.npx tsc --noEmit -p tsconfig.json✓ ·npm test→ 334/334 pass.Real vscode.dev only runs the Marketplace-published build; a local/dev build can't be sideloaded there. So this needs publishing (e.g. 1.3.7) to confirm end-to-end in production. 1.3.6 is already spent (and broken). After publish, opening a DB in vscode.dev should render the grid.
Closes #418.
Summary by CodeRabbit
Bug Fixes
Tests