diag(web): instrument in-process open sequence to locate remaining #418 hang (1.3.8) - #421
Conversation
1.3.7 removed the worker-construction crash but databases still hang on the loading screen in vscode.dev due to a second stall in the sandboxed web extension host that does NOT reproduce locally (the engine + full open sequence pass against the real test.db in a plain page) and is invisible to DevTools/network inspection. This adds per-step logging around each await in the browser establishConnection (loadDatabaseFiles, readFile(sqlite3.wasm), initializeDatabase) to the SQLite Explorer output channel AND console (captured by Extension Host (Worker) output). The last '▶ … start' with no matching '✓ … ok' pinpoints the hanging call; each step is wrapped so a thrown error surfaces as an error notification instead of an infinite spinner. Diagnostic only — no behavior change. Bumps 1.3.7 -> 1.3.8. 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 31 minutes and 20 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 (1)
📝 WalkthroughWalkthroughThis release adds detailed diagnostic logging to the web/vscode.dev database initialization sequence to diagnose the remaining "loading hang" issue ( ChangesWeb Diagnostics Instrumentation & Release
Sequence DiagramsequenceDiagram
participant establishConnection
participant diagStep
participant GlobalOutputChannel
participant vscode.window
participant endpoint
establishConnection->>diagStep: diagStep("Open DB")
diagStep->>GlobalOutputChannel: log "Open DB... start"
diagStep->>endpoint: read database bytes
endpoint-->>diagStep: bytes + timing
diagStep->>GlobalOutputChannel: log "Open DB... ok"
establishConnection->>diagStep: diagStep("Load WASM")
diagStep->>GlobalOutputChannel: log "Load WASM... start"
diagStep->>endpoint: load sqlite3.wasm
endpoint-->>diagStep: wasm bytes
diagStep->>GlobalOutputChannel: log "Load WASM... ok"
establishConnection->>diagStep: diagStep("Init engine")
diagStep->>GlobalOutputChannel: log "Init engine... start"
diagStep->>endpoint: initializeDatabase()
endpoint-->>diagStep: isReadOnly status
diagStep->>GlobalOutputChannel: log "Init engine... ok (read-only: X)"
Note over establishConnection,endpoint: On any failure: log FAILED + error to output channel, show vscode.window.showErrorMessage, re-throw
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 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 introduces diagnostic instrumentation to troubleshoot a web-only loading hang in VS Code for Web (vscode.dev). It adds logging and error handling around critical asynchronous steps during database connection establishment, specifically database file loading, WASM file reading, and database initialization. The review feedback suggests improving the diagStep helper's type signature to accept PromiseLike<T> instead of Promise<T>, which would allow passing VS Code's native Thenable objects directly and eliminate the need for manual Promise.resolve wrapping.
| GlobalOutputChannel?.appendLine(line); | ||
| console.log(line); | ||
| }; | ||
| const diagStep = async <T>(label: string, fn: () => Promise<T>): Promise<T> => { |
There was a problem hiding this comment.
Using PromiseLike<T> instead of Promise<T> in the fn signature allows the utility to accept both standard ES6 Promise objects and VS Code's Thenable objects (which are returned by VS Code workspace filesystem APIs) without requiring manual wrapping via Promise.resolve at the call sites.
| const diagStep = async <T>(label: string, fn: () => Promise<T>): Promise<T> => { | |
| const diagStep = async <T>(label: string, fn: () => PromiseLike<T>): Promise<T> => { |
| const wasmUri = vsc.Uri.joinPath(extensionUri, 'assets', 'sqlite3.wasm'); | ||
| const wasmContent = await vsc.workspace.fs.readFile(wasmUri); | ||
| diag(` wasmUri=${wasmUri.toString()}`); | ||
| const wasmContent = await diagStep('readFile(sqlite3.wasm)', () => Promise.resolve(vsc.workspace.fs.readFile(wasmUri))); |
There was a problem hiding this comment.
With diagStep updated to accept PromiseLike<T>, you can pass the Thenable returned by vsc.workspace.fs.readFile directly, removing the redundant Promise.resolve wrapper.
| const wasmContent = await diagStep('readFile(sqlite3.wasm)', () => Promise.resolve(vsc.workspace.fs.readFile(wasmUri))); | |
| const wasmContent = await diagStep('readFile(sqlite3.wasm)', () => vsc.workspace.fs.readFile(wasmUri)); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/workerFactory.ts`:
- Around line 225-229: The diag helper currently prints to console.log which
violates extension-host logging policy; update the diag function (the const diag
= (m: string) => { ... }) to stop calling console.log and route diagnostics only
through GlobalOutputChannel?.appendLine(line) (keep the existing line formatting
`[`#418` web-open] ${m}`), ensuring no other console.* calls remain in that
helper.
🪄 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: 518f2289-79a2-46e2-8756-6628548e18b1
📒 Files selected for processing (4)
.gitignoreCHANGELOG.mdpackage.jsonsrc/workerFactory.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). (2)
- GitHub Check: Build, typecheck & test
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (2)
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/workerFactory.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/workerFactory.ts
🔇 Additional comments (3)
package.json (1)
6-6: LGTM!CHANGELOG.md (1)
3-10: LGTM!.gitignore (1)
40-40: LGTM!
| const diag = (m: string) => { | ||
| const line = `[#418 web-open] ${m}`; | ||
| GlobalOutputChannel?.appendLine(line); | ||
| console.log(line); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Route diagnostics through the output channel only
The new diagnostic logger writes to console.log on Line 228, which violates extension-host logging policy for this path. Keep diagnostics on GlobalOutputChannel?.appendLine() only.
Proposed change
const diag = (m: string) => {
const line = `[`#418` web-open] ${m}`;
GlobalOutputChannel?.appendLine(line);
- console.log(line);
};As per coding guidelines, "Use GlobalOutputChannel?.appendLine() instead of console.log in the extension host for all logging to ensure output appears in the 'SQLite Explorer' output channel".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const diag = (m: string) => { | |
| const line = `[#418 web-open] ${m}`; | |
| GlobalOutputChannel?.appendLine(line); | |
| console.log(line); | |
| }; | |
| const diag = (m: string) => { | |
| const line = `[`#418` web-open] ${m}`; | |
| GlobalOutputChannel?.appendLine(line); | |
| }; |
🤖 Prompt for 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.
In `@src/workerFactory.ts` around lines 225 - 229, The diag helper currently
prints to console.log which violates extension-host logging policy; update the
diag function (the const diag = (m: string) => { ... }) to stop calling
console.log and route diagnostics only through
GlobalOutputChannel?.appendLine(line) (keep the existing line formatting `[`#418`
web-open] ${m}`), ensuring no other console.* calls remain in that helper.
#421 review) Applies Gemini's review feedback: typing diagStep's fn as PromiseLike<T> lets VS Code's Thenable (from vsc.workspace.fs.readFile) pass directly, so the Promise.resolve wrapper around the wasm readFile is removed. tsc clean. Keeps the dual output-channel + console.log diagnostic logging (CodeRabbit flagged the console.log): intentional for this throwaway diagnostic build so the trace is captured by the 'Extension Host (Worker)' output channel even if our own channel is not yet created at editor-open time. Will be removed with the diagnostic once the hang is located. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review feedback addressed (pushed in d3e76b6)Thanks @gemini-code-assist, @coderabbitai. ✅ Applied —
|
|
Thank you for the update and for providing the context on why the |
|
Tip For best results, initiate chat on the files or code changes.
Good luck with the reproduction on vscode.dev! (ᴗ_ ᴗ。) ✏️ Learnings added
|
![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: d3e76b602b
ℹ️ 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".
| }; | ||
|
|
||
| const result = await endpoint.initializeDatabase(displayName, initConfig); | ||
| const result = await diagStep('initializeDatabase (sql.js engine)', () => endpoint.initializeDatabase(displayName, initConfig)); |
There was a problem hiding this comment.
This diagnostic stops once initializeDatabase resolves, but the web loading screen still performs additional awaited backend calls immediately afterward: core/ui/viewer.js awaits backendApi.ping() and then refreshSchema(), and restored views also await fetchTableCount/fetchTableData. In a vscode.dev hang in any of those calls, the last new trace will be establishConnection complete rather than an unmatched start, so this build will not identify the remaining #418 stall it was published to diagnose. Please wrap/log the browser ping, fetchSchema, fetchTableCount, and fetchTableData facade calls as well.
Useful? React with 👍 / 👎.
| // There is no file-path fast path because the web extension host cannot | ||
| // access local disk paths directly. | ||
| const [dbContent, walContent] = await loadDatabaseFiles(fileUri); | ||
| const [dbContent, walContent] = await diagStep('loadDatabaseFiles', () => loadDatabaseFiles(fileUri)); |
There was a problem hiding this comment.
If the vscode.dev stall is inside loadDatabaseFiles, this single wrapper only reports loadDatabaseFiles … start and does not distinguish the helper's separate stat, main database readFile, and optional -wal readFile awaits (loadDatabaseFiles later waits on both reads with Promise.all). In particular, a never-settling optional WAL read would make the database open hang even after the main DB bytes are available, but this diagnostic build would not identify that call. Please add nested browser-only diagnostics around those filesystem operations.
Useful? React with 👍 / 👎.
…421 Codex review) Codex correctly flagged that the diagnostic would miss the actual #418 stall: 1. (P2) The trace stopped at initializeDatabase, but the loading screen then awaits ping -> fetchSchema -> fetchTableCount/fetchTableData (viewer.js). A hang in any of those would show 'establishConnection complete' then nothing, with no unmatched 'start'. Now the browser facade wraps ping, fetchSchema, fetchTableCount, fetchTableData, getTableInfo, getPragmas in diagStep too. 2. (P2) loadDatabaseFiles was a single wrapper hiding three awaits (stat, main readFile, optional -wal readFile via Promise.all). A never-settling -wal read over vscode-vfs would hang the open with no specific marker. loadDatabaseFiles now takes an optional diag callback and logs each fs op independently. tsc clean; build worker-free; workerFactory_browser.test.ts passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codex feedback addressed (pushed in 8ca599c)Thanks @codex — both findings were correct and would have defeated the purpose of this diagnostic. Fixed both: ✅ P2 — Instrument the post-connect RPC callsYou're right: the trace stopped at ✅ P2 — Log the individual filesystem readsAlso correct, and this is the prime suspect. Net effect: the full open path — Verification
|
Review Result
Findings Verified
Testing
|
1 participant
Summary
Diagnostic build (1.3.8) — instrumentation only, not a fix.
The 1.3.7 in-process engine fix (#420) removed the worker-construction crash (
new Worker(blobUrl)blocked by Trusted Types), confirmed in real vscode.dev: 1.3.7 loads, noTrustedScriptURLerror. But databases still hang on the loading screen — a second, environment-specific stall in the sandboxed web extension host.I can't pin it down by other means:
initializeDatabase→fetchSchema→fetchTableCount→fetchTableData) against the real 8.6 MBtest.dbin a plain browser page in <20 ms.What this adds
Per-step logging around each
awaitin the browserestablishConnection:▶ loadDatabaseFiles … start/✓ … ok (Nms)▶ readFile(sqlite3.wasm) … start/✓ … ok▶ initializeDatabase (sql.js engine) … start/✓ … okOutput goes to the SQLite Explorer output channel (revealed automatically) and
console(captured by the Extension Host (Worker) output channel), so we get the trace regardless of activation timing. The laststartwith no matchingokis the hanging call. Each step is wrapped so a thrown error surfaces as an error notification instead of an infinite spinner.Not a behavior change
Desktop path untouched. The browser path logic is unchanged except for the logging wrappers. Bumps 1.3.7 → 1.3.8.
Verification
node scripts/build.mjs✓ (browser bundle worker-free; diag marker present) ·tsc --noEmit✓ ·npm test✓ · existingworkerFactory_browser.test.tspasses with the instrumentation in place.Plan
Publish 1.3.8, open a DB in vscode.dev, read the output channel → the last logged step identifies the stalling call → real fix in a follow-up. This build will be superseded once the cause is found.
Refs #418.
Summary by CodeRabbit