feat(web): enable database editing in VS Code for Web (1.5.0)#424
Conversation
The web build was read-only since v1.0.0 (every edit threw 'Document is read-only') — a conservative original choice from when the web engine path was unreliable. Now that the in-process WASM engine works in the web extension host (1.3.9), enable read-write there. - editorController.ts: drop the !VSCODE_BROWSER_EXT term from the provider gate; rely on SupportsWriteMode (now web-aware) so the browser host gets the read-write DatabaseEditorProvider. - databaseModel.ts: SupportsWriteMode includes IsBrowserExtensionHost. save() persists web edits by serializing the DB and writing back via workspace.fs.writeFile (works on github.dev). - SAFETY: createCheckpoint() now runs only AFTER a successful write (was before), and the web write is wrapped so a rejected write throws a clear 'Failed to save database' and leaves the modification tracker uncommitted — no silent data loss; edits remain recoverable/retriable. Tests: +4 (provider selection in browser mode; SupportsWriteMode; save serialize+writeFile for non-file uri; failed-write-stays-uncommitted). The data-loss safeguard test is revert-proof-verified (fails if the checkpoint reorder is undone). Verified: build OK (extension-browser.js exports activate, worker-free); tsc --noEmit clean; npm test 338/338. NOT yet verified edit->save->persist in real vscode.dev — that requires publishing; will confirm before calling done. 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 34 minutes and 5 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. 📝 WalkthroughWalkthroughThis PR enables read-write database editing in VS Code Web/browser extension hosts by adding absolute-position checkpoint tracking, moving checkpoint creation to after persistence, detecting read-only from WAL on browser connections, wiring modification RPCs through the worker endpoint, implementing strict WASM replay with json_patch support, and expanding tests for these flows. ChangesWeb-Mode Editable Databases with Checkpointing
Sequence Diagram(s)sequenceDiagram
participant DatabaseDocument
participant WasmDatabaseEngine
participant vscode_workspace_fs as "vscode.workspace.fs"
participant ModificationTracker
DatabaseDocument->>WasmDatabaseEngine: serialize or writeToFile()
WasmDatabaseEngine->>vscode_workspace_fs: workspace.fs.writeFile / writeToFile
vscode_workspace_fs-->>WasmDatabaseEngine: success / failure
WasmDatabaseEngine->>ModificationTracker: createCheckpointAt(position) (only on success & matching invalidation revision)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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 enables database editing in VS Code for Web (vscode.dev / github.dev) by allowing write mode in the browser extension host and preventing silent data loss on save failures. It updates SupportsWriteMode to include the browser extension host, ensures the modification tracker checkpoint is only created after successful persistence, and adds comprehensive unit tests to verify these behaviors. There are no review comments, so no feedback is provided.
![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: a1cde57272
ℹ️ 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 binaryContent = await this.databaseOperations.serializeDatabase(filename); | ||
| await vsc.workspace.fs.writeFile(this.uri, binaryContent); | ||
| try { | ||
| await vsc.workspace.fs.writeFile(this.uri, binaryContent); |
There was a problem hiding this comment.
When a database opened from github.dev has a companion -wal file, loadDatabaseFiles() reads that WAL, but the WASM engine initialization never applies walContent; after this change the browser provider is writable and this line overwrites only the main database URI with the serialized in-memory DB. In that scenario, committed pages that existed only in the WAL are dropped (and the stale -wal may still remain), so saving an edited WAL-mode repository database can lose or corrupt existing data. Please checkpoint/merge the WAL or reject WAL-backed files before enabling write/save.
Useful? React with 👍 / 👎.
|
|
||
| /** Editor supports read-write operations */ | ||
| export const SupportsWriteMode = IsLocalMode || IsRemoteWorkspaceMode; | ||
| export const SupportsWriteMode = IsLocalMode || IsRemoteWorkspaceMode || IsBrowserExtensionHost; |
There was a problem hiding this comment.
With browser write mode enabled here, every web edit uses the in-process WASM facade, but that facade currently wires applyModifications, undoModification, redoModification, and discardModifications as no-ops in createInProcessWasmDatabaseConnection. In vscode.dev this means Undo/Revert and hot-exit backup restore can mark the custom document state as handled while leaving the in-memory database unchanged, so users can save edits they thought were undone or lose recovered unsaved changes. Please either implement those modification operations for the browser WASM path or keep the web provider read-only until they work.
Useful? React with 👍 / 👎.
| } | ||
| // Only mark the tracker clean after bytes are persisted. If a web filesystem | ||
| // rejects writeFile, the edit history remains uncommitted for backup/retry. | ||
| await this.#modificationTracker.createCheckpoint(); |
There was a problem hiding this comment.
Because the checkpoint is now created after the async filesystem write, any edit that arrives after serializeDatabase() has produced binaryContent but before writeFile() resolves is included in timeline.length and marked clean here even though those bytes were not part of the file that was just saved. This is especially plausible on slow web filesystem writes or with multiple editor panes, and it can silently drop the later edit on close/reload; snapshot the checkpoint position at serialization time (and only commit that snapshot after success) or block edits while saving.
Useful? React with 👍 / 👎.
Codex review found 3 confirmed data-integrity bugs that made web editing
unsafe. All fixed:
P1-A — WAL pages dropped on save. sql.js opens only the main DB image and
cannot merge a separate -wal, so saving a WAL-mode DB would overwrite the main
file and drop committed WAL pages. Now: when walContent is non-empty, the
browser connection opens read-only (viewable, not silently corruptible).
P1-B — undo/redo/revert/hot-exit were no-ops. The in-process facade wired
applyModifications/undoModification/redoModification/flushChanges/
discardModifications as async()=>{}, so VS Code Undo/Revert/backup-restore
marked document state handled while the in-memory DB was unchanged (zombie/lost
edits). createWorkerEndpoint() now exposes these ops, and the facade delegates
to the already-implemented WasmDatabaseEngine methods.
P1-C — checkpoint raced an in-flight save. createCheckpoint() ran after the
async writeFile, marking clean any edit that arrived during the write (dropped
on reload). ModificationTracker gains getCurrentPosition()/createCheckpointAt();
save() snapshots the position right after serializeDatabase() and only commits
that snapshot after a successful write. Position is absolute (timelineOffset)
so it survives front-eviction during the write.
Tests +3, each revert-proof-verified (fail when its fix is undone):
WAL-read-only, undo-delegation, save-race-stays-dirty.
Verified: build OK (extension-browser.js exports activate, worker-free);
tsc --noEmit clean; npm test 341/341. Desktop path unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
All 3 P1 data-integrity findings fixed (pushed in 5216898)Thanks @codex — all three were real and confirmed in code; web editing was not safe to ship without them. Fixed each: ✅ P1-A — WAL pages dropped on savesql.js opens only the main DB image and can't merge a separate ✅ P1-B — undo/redo/revert/hot-exit were no-opsThe in-process facade wired ✅ P1-C — checkpoint raced an in-flight save
Tests + verification+3 tests, each revert-proof-verified (fails when its fix is undone): WAL-read-only, undo-delegation, save-race-stays-dirty. |
Review Result
Testing / Checks
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52168984fe
ℹ️ 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".
| // sql.js opens one main database image and cannot merge a separate WAL | ||
| // file, so browser editing is disabled when committed WAL pages may be | ||
| // absent from the main database bytes that save() would later overwrite. | ||
| const readOnlyMode = (forceReadOnly ?? false) || hasActiveWal; |
There was a problem hiding this comment.
When hasActiveWal is true this marks only the connection/document as read-only, but the webview still uses HostBridge.isReadOnly, which returns viewerProvider.isReadOnly; in browser mode registerEditorProvider now selects DatabaseEditorProvider, so initialize() reports readOnly: false and edit methods proceed even for WAL-backed databases. Those edits mutate the in-memory sql.js database and get recorded, but save()/revert() then throw via document.isReadOnlyMode, leaving users with unsavable changes instead of a disabled editor. Please base the bridge/UI edit gate on the document connection read-only state as well, or open these WAL databases through the readonly provider.
Useful? React with 👍 / 👎.
| // rejects writeFile, the edit history remains uncommitted for backup/retry. | ||
| // The saved checkpoint is limited to the serialized snapshot so concurrent | ||
| // edits are not acknowledged before their bytes reach storage. | ||
| await this.#modificationTracker.createCheckpointAt(serializedCheckpoint); |
There was a problem hiding this comment.
If the user invokes Undo/Redo while the web writeFile() is still pending, the serialized bytes no longer match the in-memory document state. For example, saving after edits A+B serializes A+B, then Undo removes B before this line runs; createCheckpointAt() clamps the saved position to the shortened timeline and marks A as clean even though the file now contains A+B, so closing/reloading silently loses the user's undo. Fresh evidence beyond the earlier edit-race comment is that timeline-shrinking operations during the same in-flight save bypass the captured-position fix; block undo/redo while saving or only checkpoint when the current document state still matches the serialized snapshot.
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/undo-history.ts (1)
22-56: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUint8Array serialization key mismatch with coding guidelines.
The serialization uses
{ __type: 'Uint8Array', data: base64 }but the coding guidelines specify{ __type: 'Uint8Array', base64: '...' }. This inconsistency could cause interoperability issues if other parts of the codebase expect thebase64key.♻️ Proposed fix to align with coding guidelines
function binaryReplacer(_key: string, value: unknown): unknown { if (value instanceof Uint8Array) { // Convert to base64 with a type marker for restoration // Use Buffer in Node.js environment for efficiency const base64 = typeof Buffer !== 'undefined' ? Buffer.from(value).toString('base64') : btoa(String.fromCharCode(...value)); - return { __type: 'Uint8Array', data: base64 }; + return { __type: 'Uint8Array', base64 }; } return value; }function binaryReviver(_key: string, value: unknown): unknown { if (value && typeof value === 'object' && '__type' in value) { - const typed = value as { __type: string; data: string }; - if (typed.__type === 'Uint8Array' && typeof typed.data === 'string') { + const typed = value as { __type: string; base64: string }; + if (typed.__type === 'Uint8Array' && typeof typed.base64 === 'string') { // Restore from base64 if (typeof Buffer !== 'undefined') { - return new Uint8Array(Buffer.from(typed.data, 'base64')); + return new Uint8Array(Buffer.from(typed.base64, 'base64')); } else { - const binary = atob(typed.data); + const binary = atob(typed.base64); const bytes = new Uint8Array(binary.length); for (let i = 0; i < binary.length; i++) { bytes[i] = binary.charCodeAt(i); } return bytes; } } } return value; }As per coding guidelines: "Serialize
Uint8Arrayusing the marker format{ __type: 'Uint8Array', base64: '...' }with exactly 2 keys".🤖 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/core/undo-history.ts` around lines 22 - 56, The serializer uses { __type: 'Uint8Array', data: base64 } but the codebase requires the marker be { __type: 'Uint8Array', base64: '...' } with exactly two keys; update binaryReplacer to return { __type: 'Uint8Array', base64: base64 } instead of using `data`, and update binaryReviver to check for the `base64` property (typeof string) and restore from that field; ensure both functions only rely on __type and base64 so the restored object matches the guideline.src/workerFactory.ts (1)
339-339:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffFix worker-backed WASM modification operations being no-ops
src/core/sqlite-db.ts’s worker endpoint exportsapplyModifications,undoModification,redoModification,flushChanges, anddiscardModifications, butsrc/workerFactory.tsomits them from theconnectWorkerPort(..., [...])proxy allowlist (~line 339) and the worker-backedDatabaseOperationsfacade implements these as emptyasync () => {}stubs (~lines 442-446). On desktop falling back to the worker-backed WASM path, undo/redo/flush/discard silently won’t work.🤖 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` at line 339, The worker proxy allowlist in connectWorkerPort and the DatabaseOperations facade need to include and forward the WASM modification APIs so undo/redo/flush/discard aren't no-ops: add 'applyModifications', 'undoModification', 'redoModification', 'flushChanges', and 'discardModifications' to the array passed to connectWorkerPort in workerFactory.ts and replace the empty async stubs in the DatabaseOperations implementation with proper proxied calls that invoke the corresponding worker-exported functions (applyModifications, undoModification, redoModification, flushChanges, discardModifications) so the worker-backed path performs the real operations.
🤖 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 `@tests/unit/databaseModel.test.ts`:
- Around line 14-48: The three duplicate loaders (loadBrowserDatabaseModel,
loadBrowserEditorController, loadBrowserWorkerFactory) should be consolidated
into a shared helper (e.g., loadBrowserModule) that accepts the module absolute
path and a requireImpl callback; replace the duplicated esbuild transform,
Module creation/_nodeModulePaths setup, Module.prototype.require monkeypatch and
_compile/finally teardown with a single function signature like
loadBrowserModule(absPath, requireImpl) and update each test to call it, passing
the per-test require overrides (the current inline callbacks that return
mockVscode, workerFactory, main, etc.) so behavior remains identical while
removing copy/paste.
- Around line 423-432: Replace the direct reassignment of
mockVscode.workspace.fs with Object.defineProperty to set the readonly VS Code
API field; specifically, instead of mockVscode.workspace.fs = { ... }, call
Object.defineProperty(mockVscode.workspace, 'fs', { value: { ...originalFs,
writeFile: async (uri, content) => { ... }, readFile: async () => ... },
writable: true, configurable: true }); Do the same replacement for the other two
fs overrides in this test file (the ones creating the alternative
readFile/writeFile mocks) so all workspace.fs assignments use
Object.defineProperty and remain writable/configurable.
---
Outside diff comments:
In `@src/core/undo-history.ts`:
- Around line 22-56: The serializer uses { __type: 'Uint8Array', data: base64 }
but the codebase requires the marker be { __type: 'Uint8Array', base64: '...' }
with exactly two keys; update binaryReplacer to return { __type: 'Uint8Array',
base64: base64 } instead of using `data`, and update binaryReviver to check for
the `base64` property (typeof string) and restore from that field; ensure both
functions only rely on __type and base64 so the restored object matches the
guideline.
In `@src/workerFactory.ts`:
- Line 339: The worker proxy allowlist in connectWorkerPort and the
DatabaseOperations facade need to include and forward the WASM modification APIs
so undo/redo/flush/discard aren't no-ops: add 'applyModifications',
'undoModification', 'redoModification', 'flushChanges', and
'discardModifications' to the array passed to connectWorkerPort in
workerFactory.ts and replace the empty async stubs in the DatabaseOperations
implementation with proper proxied calls that invoke the corresponding
worker-exported functions (applyModifications, undoModification,
redoModification, flushChanges, discardModifications) so the worker-backed path
performs the real operations.
🪄 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: b3ec4376-a595-45ec-9e8d-6f3ac3d44ca0
📒 Files selected for processing (10)
CHANGELOG.mdpackage.jsonsrc/core/sqlite-db.tssrc/core/undo-history.tssrc/databaseModel.tssrc/editorController.tssrc/workerFactory.tstests/unit/databaseModel.test.tstests/unit/editorController.test.tstests/unit/workerFactory_browser.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 (4)
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/editorController.tssrc/core/sqlite-db.tssrc/workerFactory.tssrc/core/undo-history.tssrc/databaseModel.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/editorController.tssrc/core/sqlite-db.tssrc/workerFactory.tssrc/core/undo-history.tssrc/databaseModel.ts
src/editorController.ts
📄 CodeRabbit inference engine (CLAUDE.md)
src/editorController.ts: UseretainContextWhenHidden: falsefor webview configuration to destroy webviews when hidden and preserve state viavscodeApi.setState()/getState()
Use nonce-based Content Security Policy for scripts (no'unsafe-inline') and for<style>tags in the webview
Implement CustomEditorProvider withopenCustomDocument()to create DatabaseDocument,resolveCustomEditor()to create webview and RPC, andsave()/dispose()lifecycle methods
Files:
src/editorController.ts
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/editorController.test.tstests/unit/workerFactory_browser.test.tstests/unit/databaseModel.test.ts
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~7-~7: The official name of this software platform is spelled with a capital “H”.
Context: ...abases in VS Code for Web (vscode.dev / github.dev)**. The web build was previously re...
(GITHUB)
[uncategorized] ~7-~7: The official name of this software platform is spelled with a capital “H”.
Context: ...filesystem (for example, committing via github.dev). ### Safety - **No silent data l...
(GITHUB)
🔇 Additional comments (15)
package.json (1)
6-6: LGTM!CHANGELOG.md (1)
3-12: LGTM!src/core/undo-history.ts (1)
145-146: LGTM!Also applies to: 213-213, 267-286
src/core/sqlite-db.ts (1)
24-25: LGTM!Also applies to: 178-198
src/databaseModel.ts (2)
47-49: LGTM!Also applies to: 56-56
326-375: LGTM!src/workerFactory.ts (1)
217-221: LGTM!Also applies to: 234-234, 245-254
src/editorController.ts (1)
385-390: LGTM!tests/unit/editorController.test.ts (2)
43-82: ThisloadBrowserEditorControllerloader duplicates the same esbuild/Modulemonkeypatch machinery asloadBrowserDatabaseModelindatabaseModel.test.ts(and the pre-existing loader inworkerFactory_browser.test.ts). See the shared-helper suggestion raised there.
134-150: LGTM!tests/unit/databaseModel.test.ts (3)
57-100: LGTM!
253-266: LGTM!
444-565: LGTM!tests/unit/workerFactory_browser.test.ts (2)
10-23: LGTM!
176-303: LGTM!
| function loadBrowserDatabaseModel() { | ||
| const jsCode = esbuild.transformSync(databaseModelSource, { | ||
| loader: 'ts', | ||
| format: 'cjs', | ||
| define: { | ||
| 'import.meta.env.VSCODE_BROWSER_EXT': 'true' | ||
| } | ||
| }).code; | ||
|
|
||
| const scriptModule = new Module(databaseModelPath, module as unknown as Module); | ||
| scriptModule.filename = databaseModelPath; | ||
| scriptModule.paths = (Module as unknown as { _nodeModulePaths(dirname: string): string[] }) | ||
| ._nodeModulePaths(path.dirname(databaseModelPath)); | ||
|
|
||
| const originalRequire = Module.prototype.require; | ||
| Module.prototype.require = function(request: string) { | ||
| if (request === 'vscode') return mockVscode; | ||
| if (request.endsWith('workerFactory')) { | ||
| return { createDatabaseConnection: async () => ({}) }; | ||
| } | ||
| if (request.endsWith('main')) { | ||
| return { GlobalOutputChannel: null }; | ||
| } | ||
| return originalRequire.call(this, request); | ||
| }; | ||
|
|
||
| try { | ||
| (scriptModule as unknown as { _compile(code: string, filename: string): void }) | ||
| ._compile(jsCode, databaseModelPath); | ||
| } finally { | ||
| Module.prototype.require = originalRequire; | ||
| } | ||
|
|
||
| return scriptModule.exports as typeof import('../../src/databaseModel'); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider extracting a shared browser-module loader.
loadBrowserDatabaseModel() is now nearly identical to loadBrowserEditorController() (editorController.test.ts) and loadBrowserWorkerFactory() (workerFactory_browser.test.ts): same esbuild transform, same import.meta.env.VSCODE_BROWSER_EXT define, same new Module + _nodeModulePaths + Module.prototype.require monkeypatch + _compile in try/finally. Only the source path and the require overrides differ. Extracting a helper such as loadBrowserModule(absPath, requireImpl) would prevent these copies from drifting.
♻️ Sketch of a shared helper
// tests/unit/helpers/loadBrowserModule.ts
export function loadBrowserModule<T>(
absPath: string,
requireImpl: (request: string, original: NodeRequire, self: any) => unknown | undefined
): T {
const jsCode = esbuild.transformSync(fs.readFileSync(absPath, 'utf8'), {
loader: 'ts',
format: 'cjs',
define: { 'import.meta.env.VSCODE_BROWSER_EXT': 'true' }
}).code;
const scriptModule = new Module(absPath, module as unknown as Module);
scriptModule.filename = absPath;
scriptModule.paths = (Module as unknown as { _nodeModulePaths(d: string): string[] })
._nodeModulePaths(path.dirname(absPath));
const originalRequire = Module.prototype.require;
Module.prototype.require = function (request: string) {
const mocked = requireImpl(request, originalRequire as NodeRequire, this);
return mocked !== undefined ? mocked : originalRequire.call(this, request);
};
try {
(scriptModule as unknown as { _compile(c: string, f: string): void })._compile(jsCode, absPath);
} finally {
Module.prototype.require = originalRequire;
}
return scriptModule.exports as T;
}🤖 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 `@tests/unit/databaseModel.test.ts` around lines 14 - 48, The three duplicate
loaders (loadBrowserDatabaseModel, loadBrowserEditorController,
loadBrowserWorkerFactory) should be consolidated into a shared helper (e.g.,
loadBrowserModule) that accepts the module absolute path and a requireImpl
callback; replace the duplicated esbuild transform, Module
creation/_nodeModulePaths setup, Module.prototype.require monkeypatch and
_compile/finally teardown with a single function signature like
loadBrowserModule(absPath, requireImpl) and update each test to call it, passing
the per-test require overrides (the current inline callbacks that return
mockVscode, workerFactory, main, etc.) so behavior remains identical while
removing copy/paste.
…nd 2)
Re-review of the P1 fixes found two more issues, both in those fixes:
1. WAL read-only guard was incomplete (P1). The browser WAL guard marked the
connection/document read-only, but hostBridge.isReadOnly only checked the
provider (DatabaseEditorProvider, isReadOnly=false in web), so the webview
still allowed edits on a WAL database that save()/revert() then rejected —
a dead-end with unsavable changes. hostBridge.isReadOnly now also honors
document.isReadOnlyMode, so the edit gate is consistent with the connection.
2. undo/redo DURING an in-flight save could still lose data (P2). The
serialized-position snapshot handled edits ADDED during the async write but
not timeline SHRINKING: if the user undoes an edit while writeFile is
pending, createCheckpointAt clamps to the shortened timeline and marks bytes
clean that no longer match the in-memory state, silently losing the undo on
reload. ModificationTracker now tracks a checkpointInvalidationRevision
(bumped by undo/rollback/eviction); save() captures it before the write and
only commits the checkpoint if it is unchanged afterward — otherwise the
document stays dirty so the next save reconciles. Normal desktop save (no
concurrent undo) is unaffected.
Also fixed a stale test: editorController.test.ts passed {} as the mock
document; with hostBridge.isReadOnly now reading document.isReadOnlyMode, the
mock must expose it (isReadOnlyMode: false).
Tests +2, both revert-proof-verified. tsc clean; npm test 343/343. Desktop
unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-2 findings fixed (pushed in 768c679)Both new findings from the re-review were real and in the prior fixes. Fixed: ✅ P1 — WAL read-only guard was incomplete (Codex, workerFactory.ts:221)The WAL guard set the connection/document read-only, but ✅ P2 — undo/redo during an in-flight save (Codex, databaseModel.ts:374)The serialized-position snapshot handled edits added during the async write but not timeline shrinking: undo while CodeRabbit nitpicks
Also fixed a stale test ( Tests +2, both revert-proof-verified. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 768c679967
ℹ️ 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 (this.uri.scheme === 'file') { | ||
| try { | ||
| await this.databaseOperations.writeToFile(this.uri.fsPath); | ||
| await this.#modificationTracker.createCheckpoint(); |
There was a problem hiding this comment.
When a WASM-backed file:// document uses the direct writeToFile() path, that helper exports the sql.js bytes and then awaits fs.promises.writeFile; if another editor pane records an edit or undo while that write is pending, the new state is not in the bytes that were just persisted, but this line checkpoints the current tracker and marks it clean. The captured-position guard below only protects the fallback workspace.fs.writeFile path, so this branch can still drop edits made during an in-flight direct save; capture the tracker position before writeToFile() starts or block edits while this save is active.
Useful? React with 👍 / 👎.
| applyModifications: (mods: ModificationEntry[], signal?: AbortSignal) => | ||
| endpoint.applyModifications(mods, signal), |
There was a problem hiding this comment.
This browser facade now forwards hot-exit restore through endpoint.applyModifications, but fresh evidence is that the endpoint still lands in WasmDatabaseEngine.applyModifications, whose implementation is a no-op. In vscode.dev, reopening a dirty database from a backup calls DatabaseDocument.create() to replay tracker.getUncommittedEntries() through this path; the document then appears restored while the in-memory database still contains the last saved bytes, so a subsequent save can silently discard the recovered edits. Please implement replay for each ModificationEntry before wiring this method for web writes.
Useful? React with 👍 / 👎.
…ce (#424 review round 3) Two more confirmed data-integrity issues, both in/under the prior fixes: 1. (P1) WasmDatabaseEngine.applyModifications was an empty no-op, so the round-1 'fix' that delegated the in-process facade to it did nothing real: hot-exit backup restore (DatabaseDocument.create replays tracker.getUncommittedEntries() via applyModifications) silently dropped the recovered edits — the doc looked restored but the in-memory DB held the last saved bytes, and the next save discarded the edits. applyModifications now replays each entry forward. Extracted a shared private forwardApply(mod, strict) used by BOTH applyModifications (strict: throws on malformed/ unsupported entries so restore fails loud) and redoModification (non-strict: unchanged historical behavior). Covers all ModificationType values; respects the abort signal between entries. 2. (P2) The file:// direct-save branch (writeToFile) still used a plain createCheckpoint after the async write, so a concurrent edit/undo during an in-flight direct save could mark mismatched bytes clean. It now uses the same capture-position + checkpointInvalidationRevision guard as the workspace.fs.writeFile branch (commit checkpoint only if the timeline didn't change during the write; else stay dirty). Tests +4 (applyModifications actually replays; restore replays a dirty backup; restore aborts on a pre-aborted signal; file:// save stays dirty on concurrent undo). Both fixes revert-proof-verified. tsc clean; npm test 347/347. redo and desktop behavior unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-3 findings fixed (pushed in 136ef23)Both new findings confirmed and fixed. (The other 5 inline comments this round were stale re-anchors of already-fixed round-1/2 items — verified the fixes are present in the current code.) ✅ P1 — applyModifications was a no-op (undermined the earlier delegation fix)
✅ P2 — file:// direct-save shared the save-raceThe Verification+4 tests, both fixes revert-proof-verified (applyModifications-replay and file:// race both fail when reverted). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 136ef2329c
ℹ️ 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 updates = affectedCells.map(cell => ({ | ||
| rowId: cell.rowId, | ||
| column: cell.columnName, |
There was a problem hiding this comment.
When a multi-cell JSON edit is optimized in HostBridge.updateCellBatch, the history entry records newValue as the generated merge-patch string rather than the final cell contents, and this newly wired browser WASM replay/redo path turns that field into a normal set update. In vscode.dev, Undo→Redo or hot-exit restore for such a batch edit will write the patch document itself into the cell instead of reapplying the JSON change; preserve the operation/final value in history or replay these entries as JSON patches.
Useful? React with 👍 / 👎.
| if (deletedColumns) { | ||
| const colNames = deletedColumns.map(c => c.name); | ||
| await this.deleteColumns(targetTable, colNames); |
There was a problem hiding this comment.
When the original column drop required confirming dependent indexes, HostBridge.deleteColumns only records the deleted column data in history while the index names are passed separately to deleteColumns for the live edit. This newly wired WASM replay path calls deleteColumns without those indexes, so hot-exit restore or browser redo of that edit will hit SQLite's dependent-index failure and leave the backup unrecovered/read-only; record and replay the dropped index names or rediscover them before dropping the columns.
Useful? React with 👍 / 👎.
| signal?.throwIfAborted(); | ||
| for (const mod of mods) { | ||
| await this.forwardApply(mod, true); | ||
| signal?.throwIfAborted(); |
There was a problem hiding this comment.
If restore cancellation or a later unsupported/malformed edit occurs after one or more entries have been applied, this loop leaves those earlier changes in the in-memory database before DatabaseDocument.create() catches the error and opens the document read-only. In that scenario the restored database no longer matches either the saved file or the full backup history, and users can inspect/export a partially replayed state; apply the batch inside a transaction/savepoint and roll it back on any error or abort.
Useful? React with 👍 / 👎.
…d 4)
All three were lossy/incorrect forward-replay of serialized edit history during
hot-exit restore (the round-3 replay path):
1. (P1) JSON-patch batch/single cell updates replayed as raw value SETS, not
patches, because the history entry recorded no per-cell operation and the
pre-processed value. ModificationEntry + affectedCells now carry an
'operation' ('set'|'json_patch'); HostBridge.updateCell/updateCellBatch
record it (and the patch as newValue) from the processed updates; forwardApply
replays json_patch cells via updateCell's patch arg / CellUpdate.operation, so
restore reproduces the merged JSON instead of overwriting.
2. (P1) column_drop replay omitted dependent indexes → replay threw when an index
still referenced the column. ModificationEntry now records droppedIndexes;
HostBridge.deleteColumns stores them; forwardApply passes them to
deleteColumns(table, cols, droppedIndexes).
3. (P2) Partial restore wasn't atomic: a malformed/aborted entry mid-loop left a
half-applied DB. applyModifications now wraps the whole replay in a SAVEPOINT
(RELEASE on success, ROLLBACK TO + RELEASE on any failure/abort) so restore is
all-or-nothing, leaving the DB at the last-saved bytes on failure. Required
converting deleteColumns' inner BEGIN TRANSACTION to a SAVEPOINT so it nests
inside the restore transaction (SQLite can't nest BEGIN). insertRowBatch /
undoColumnDrop BEGINs are not reachable from forwardApply, so left as-is.
Backward-compatible: missing operation -> 'set', missing droppedIndexes -> none.
redo (forwardApply non-strict) + desktop/native + worker paths unchanged.
Tests +3 (json_patch batch round-trip; column_drop+index replay; atomic
rollback on malformed entry), all revert-proof-verified. tsc clean; npm test
350/350.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-4 findings fixed (pushed in 3172d6d)All three new findings confirmed and fixed. (Other inline comments this round were stale re-anchors of fixed round-1/2/3 items — verified against current code.) ✅ P1 — JSON-patch batch/cell updates replayed as raw value setsHistory recorded no per-cell ✅ P1 — column_drop replay omitted dependent indexesRecorded ✅ P2 — partial restore wasn't atomic
Backward-compatible (missing +3 tests, all revert-proof-verified. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3172d6dd67
ℹ️ 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".
| targetRowId: rowId, | ||
| targetColumn: column, | ||
| newValue: value, | ||
| newValue: patch ?? value, |
There was a problem hiding this comment.
When tryGeneratePatch succeeds, this records the merge-patch document as newValue for every engine, but the native redo path still ignores operation and writes newValue directly (src/nativeWorker.ts:615-636). In a normal local/native session, editing a JSON object, then Undo → Redo will store the patch (for example {"field":2}) instead of the full final cell contents, so history corrupts the cell even though the live edit initially wrote the correct value; keep the final value in history for native redo or teach native redo to apply json_patch.
Useful? React with 👍 / 👎.
| resourceMap: {}, | ||
| wasmBinary: wasmContent, | ||
| readOnlyMode: forceReadOnly ?? false, | ||
| readOnlyMode, |
There was a problem hiding this comment.
When a browser database has an active -wal, this only propagates the read-only state as metadata; the underlying operations object is still writable. The separate cell-editor filesystem path bypasses HostBridge.isReadOnly and calls document.databaseOperations.updateCell directly (src/virtualFileSystem.ts:131), so opening a cell in VS Code and saving it can still mutate the in-memory WAL-backed database and record dirty edits that save()/revert() later reject as read-only. Please enforce the read-only state in the operations/VFS path or mark normal cell URIs readonly too.
Useful? React with 👍 / 👎.
| updateCell: (table: string, rowId: string | number, column: string, value: CellValue) => | ||
| endpoint.updateCell(table, rowId, column, value), |
There was a problem hiding this comment.
With browser write mode enabled, single-cell JSON edits call HostBridge.updateCell(..., value, patch), but this in-process facade only accepts four arguments and forwards only value, dropping the generated merge patch before it reaches WasmDatabaseEngine.updateCell. In vscode.dev, if the cell's JSON has changed since the webview's originalValue was captured (for example another editor pane changed a different key), the edit overwrites the whole stale JSON document instead of merging the patch; include the optional patch parameter here and forward it to the endpoint.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hostBridge.ts (1)
389-440:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize batch
operationvalues before persisting them.Line 390 only special-cases
'json_patch', and Line 440 stores any other incoming value verbatim.WasmDatabaseEngine.normalizeReplayCellOperation()later throws on anything outside'set' | 'json_patch', so a malformed or stale webview payload can poison undo/hot-exit replay. Coerce unknown values to'set'or reject them before forwarding and recording the batch.Suggested fix
async updateCellBatch(table: string, updates: CellUpdate[], label: string) { const dbOps = this.ensureDatabaseInitialized(); @@ - const processedUpdates = updates.map(update => { - if (update.operation !== 'json_patch') { + const normalizeOperation = (operation: CellUpdate['operation']): 'set' | 'json_patch' => + operation === 'json_patch' ? 'json_patch' : 'set'; + + const processedUpdates = updates.map(update => { + const operation = normalizeOperation(update.operation); + if (operation !== 'json_patch') { const patch = this.tryGeneratePatch(update.value, update.originalValue); if (patch) { return { ...update, operation: 'json_patch' as const, value: patch }; } } - return update; + return { + ...update, + operation + }; }); @@ affectedCells: processedUpdates.map(u => ({ rowId: u.rowId, columnName: u.column, newValue: u.value, priorValue: u.originalValue, - operation: u.operation ?? 'set' + operation: u.operation })) }); }🤖 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/hostBridge.ts` around lines 389 - 440, Batch update handling currently only preserves 'json_patch' and leaves any other operation verbatim, which later causes normalizeReplayCellOperation to throw on unknown values; update the processing so each update's operation is normalized to either 'json_patch' or 'set' before any DB calls or before building affectedCells. Concretely, in the processedUpdates mapping (and any fallback loop using update.operation) coerce operation to (update.operation === 'json_patch' ? 'json_patch' : 'set'), ensure you pass the normalized operation into dbOps.updateCell/updateCellBatch and use that same normalized operation when constructing the affectedCells array (the mapping that uses u.operation) so only 'set' or 'json_patch' are persisted/replayed; alternatively, validate and reject unknown operation values earlier if you prefer failing fast.
🤖 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/core/engine/wasm/WasmDatabaseEngine.ts`:
- Around line 235-237: The loop over mods begins work before checking for
cancellation; add a pre-step cancellation check by calling
signal?.throwIfAborted() at the top of the for (const mod of mods) loop so
cancellation is observed before invoking forwardApply(mod, true); keep the
existing post-call check in place; reference the loop variables mods, signal and
the forwardApply method to locate where to insert the check.
In `@src/core/undo-history.ts`:
- Around line 257-258: The backup serialization currently only writes timeline
and checkpointIndex so undos in futureStack (moved by stepBack() and
rollbackToCheckpoint()) are lost; update serialize() to also persist futureStack
and the current live cursor/state (e.g., currentEntryIndex or a liveState
snapshot) and update restore() to read futureStack and either restore the live
DB snapshot or reconstruct the cursor by applying timeline +
getUncommittedEntries() plus the persisted futureStack to recreate the exact
live/future stacks; alternatively persist a full snapshot of live DB state and
restore it directly so undone state survives hot-exit.
In `@tests/unit/sqlite_db.test.ts`:
- Around line 2-5: Add a top-of-file import for the shared VS Code mock
bootstrap so mocks are initialized before the test module loads: prepend an
import of the test bootstrap (tests/unit/vscode_mock_setup.ts) at the very
beginning of this test file (before the existing imports such as
createDatabaseEngine, DatabaseOperations, CellUpdate, ModificationEntry) to
ensure the VS Code mock setup runs first.
---
Outside diff comments:
In `@src/hostBridge.ts`:
- Around line 389-440: Batch update handling currently only preserves
'json_patch' and leaves any other operation verbatim, which later causes
normalizeReplayCellOperation to throw on unknown values; update the processing
so each update's operation is normalized to either 'json_patch' or 'set' before
any DB calls or before building affectedCells. Concretely, in the
processedUpdates mapping (and any fallback loop using update.operation) coerce
operation to (update.operation === 'json_patch' ? 'json_patch' : 'set'), ensure
you pass the normalized operation into dbOps.updateCell/updateCellBatch and use
that same normalized operation when constructing the affectedCells array (the
mapping that uses u.operation) so only 'set' or 'json_patch' are
persisted/replayed; alternatively, validate and reject unknown operation values
earlier if you prefer failing fast.
🪄 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: f266b6a1-4a2c-462b-b224-e1b614e032e6
📒 Files selected for processing (10)
src/core/engine/wasm/WasmDatabaseEngine.tssrc/core/types.tssrc/core/undo-history.tssrc/databaseModel.tssrc/hostBridge.tstests/unit/databaseModel.test.tstests/unit/editorController.test.tstests/unit/hostBridge.test.tstests/unit/hot_exit_restore_replay.test.tstests/unit/sqlite_db.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
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/hostBridge.test.tstests/unit/hot_exit_restore_replay.test.tstests/unit/sqlite_db.test.tstests/unit/editorController.test.tstests/unit/databaseModel.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/core/types.tssrc/core/undo-history.tssrc/core/engine/wasm/WasmDatabaseEngine.tssrc/hostBridge.tssrc/databaseModel.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/core/types.tssrc/core/undo-history.tssrc/core/engine/wasm/WasmDatabaseEngine.tssrc/hostBridge.tssrc/databaseModel.ts
{src/hostBridge.ts,core/ui/modules/settings.js}
📄 CodeRabbit inference engine (CLAUDE.md)
Provide a UI to configure SQLite PRAGMAs via
hostBridge.setPragma()to allow users to modify settings like WAL mode and Foreign Keys directly
Files:
src/hostBridge.ts
🔇 Additional comments (8)
src/core/engine/wasm/WasmDatabaseEngine.ts (1)
100-131: LGTM!Also applies to: 393-489, 719-739
tests/unit/sqlite_db.test.ts (1)
122-186: LGTM!tests/unit/hostBridge.test.ts (1)
151-196: LGTM!tests/unit/hot_exit_restore_replay.test.ts (1)
1-153: LGTM!src/core/types.ts (1)
138-141: LGTM!Also applies to: 161-174, 190-191, 294-294
src/hostBridge.ts (1)
124-145: LGTM!Also applies to: 347-348, 612-621
tests/unit/editorController.test.ts (1)
148-150: LGTM!tests/unit/databaseModel.test.ts (1)
624-649: UseObject.definePropertyfor the newworkspace.fsoverrides.These new tests still replace a readonly VS Code API field via direct assignment. Please switch these overrides/restores to
Object.defineProperty(..., { value, writable: true, configurable: true })the same way as the other readonly mock fields.As per coding guidelines: "Use
Object.defineProperty(obj, prop, { value, writable: true, configurable: true })for setting readonly VS Code API fields likevscode.env.uiKindin unit tests."Also applies to: 778-823
| this.invalidateCapturedCheckpointPositions(); | ||
| } |
There was a problem hiding this comment.
Persist undone state in backups as well.
stepBack() and rollbackToCheckpoint() now move live state into futureStack, but serialize() still writes only timeline and checkpointIndex, and restore only replays getUncommittedEntries(). If a user undoes a saved edit and then hot-exits, reopen will restore the saved DB state instead of the current undone state.
Please persist enough state to reconstruct the current cursor position on restore, or snapshot the live DB state for backups instead of replaying only forward entries.
Also applies to: 358-358
🤖 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/core/undo-history.ts` around lines 257 - 258, The backup serialization
currently only writes timeline and checkpointIndex so undos in futureStack
(moved by stepBack() and rollbackToCheckpoint()) are lost; update serialize() to
also persist futureStack and the current live cursor/state (e.g.,
currentEntryIndex or a liveState snapshot) and update restore() to read
futureStack and either restore the live DB snapshot or reconstruct the cursor by
applying timeline + getUncommittedEntries() plus the persisted futureStack to
recreate the exact live/future stacks; alternatively persist a full snapshot of
live DB state and restore it directly so undone state survives hot-exit.
…review round 5)
ModificationEntry was forward-applied by three divergent implementations
(live edit via HostBridge, web redo via WasmDatabaseEngine.forwardApply,
native redo via nativeWorker.redoModification) that interpreted the same
fields differently — the root cause of recurring replay data bugs across
review rounds.
Native redoModification reimplemented raw SQL and ignored both `operation`
and `droppedIndexes`. Combined with the earlier branch change that records
`newValue: patch` + `operation` for json_patch cells, this corrupted desktop
redo: a json_patch cell redo wrote the raw patch string as a literal value.
Unify by delegating native redo to the same operationsFacade primitives the
live-edit path uses, so ModificationEntry has ONE interpretation everywhere:
- cell_update batch -> updateCellBatch (honors per-cell operation)
- cell_update single -> updateCell (json_patch -> patch arg; else value)
- row_insert/delete -> insertRow / deleteRows
- column_add -> addColumn (same DEFAULT-value handling)
- column_drop -> deleteColumns(table, cols, droppedIndexes) drops
dependent indexes first
- table_create -> createTable (same primaryKey/notNull handling)
Native redo now structurally mirrors WasmDatabaseEngine.forwardApply
(non-strict); paired-contract comments in both point at each other so they
cannot drift again.
Tests: 3 regression tests driving the real native length-prefixed V8 IPC,
asserting json_patch single (COALESCE patch SQL, not a literal SET), batch
(operation-aware execBatch grouping), and column_drop (DROP INDEX before
ALTER DROP COLUMN). Revert-proof: all 3 fail against the old raw-SQL redo.
Full suite 353/353 (+3); build + tsc clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review round 5 — root-cause unification (commit 6b3bc06)Rounds 3–5 kept surfacing replay data bugs because
This also caused a desktop regression introduced earlier on this branch: once Fix: one forward-apply contractNative
Native redo now structurally mirrors Tests (3 new, revert-proof)Drive the real native length-prefixed V8 IPC and assert emitted SQL:
Revert-proof verified: all 3 fail against the old raw-SQL redo, pass with the fix. Verification (run locally, real env)
Known nit (pre-existing, out of scope, not a regression)Batch json_patch uses |
|
@coderabbitai review |
|
/gemini review |
…kpoint (#424) Both bots flagged the same data bug independently (CodeRabbit major, Gemini): - nativeWorker.updateCellBatch json_patch used `json_patch(col, ?)` without COALESCE — the SOLE json_patch site missing it (native single-cell, WASM single, and WASM batch all use `json_patch(COALESCE(col,'{}'), ?)`). A batch json_patch on a NULL cell returned NULL, silently discarding the patch. This affects the LIVE desktop batch path, not just redo. Now matches the other three sites. Batch redo test updated to assert the COALESCE form (+ a notStrictEqual guard); revert-proof verified. Gemini (medium): - ModificationTracker.createCheckpointAt was `async`/`Promise<void>` but does only synchronous work (arithmetic + array bounds). Made synchronous and dropped the two unnecessary `await`s in databaseModel (the only callers). CodeRabbit (refactor): - tests/unit/sqlite_db.test.ts now imports `./vscode_mock_setup` first per the documented unit-test convention (defensive: it passes standalone today since sqlite-db has no vscode dependency, but this removes suite-order fragility). tsc clean; build OK; full suite 353/353. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 5 feedback addressed (commit 9e1bf37)Verified each finding on its merits before applying — all three were legitimate. 🟠 batch 🟡 🛠️ Verification (local, real env): The pre-existing single-vs-batch asymmetry I flagged last round is now resolved (both use COALESCE). No outstanding findings from this round on my side. |
|
@coderabbitai review |
|
/gemini review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request introduces database editing capabilities in VS Code for Web (vscode.dev / github.dev) by enabling the read-write editor in the browser extension host. Key changes include implementing hot-exit restore by replaying uncommitted edit history into the in-memory WASM database, introducing robust savepoint-based transaction handling, and preventing silent data loss on web save failures by keeping the edit history uncommitted if a write fails. Additionally, it adds support for json_patch cell updates, handles active WAL files by opening them as read-only in the browser, and includes comprehensive unit tests validating these new behaviors. There are no review comments, so no further feedback is provided.
…llation (#424 review round 6) Three still-valid findings from the round-6 bot review (CodeRabbit + Codex connector); other flagged items were verified already-fixed in earlier rounds. P2 — Forward the json_patch arg through the in-process / browser facades. WasmDatabaseEngine.updateCell accepts (…, value, patch?) and merges when patch is set, but the patch was silently dropped at createWorkerEndpoint.updateCell (sqlite-db.ts) and the browser in-process facade (workerFactory.ts). The worker-backed desktop facade dropped it too. All now forward patch (interface WorkerMethods.updateCell widened). Without this, a single-cell JSON edit in vscode.dev (and desktop wasm mode) overwrites the whole — possibly stale — document instead of applying the merge patch. P2 — Enforce read-only in the cell-editor (VFS) write path. SQLiteFileSystemProvider.writeFile called updateCell with no read-only check, so a WAL-backed (read-only) browser database could be mutated via the cell editor, recording dirty edits save()/revert() later reject. Now throws FileSystemError.NoPermissions when document.isReadOnlyMode, before decoding content or mutating SQLite. Minor — Check cancellation before each replay step. applyModifications now calls signal?.throwIfAborted() at the top of the replay loop as well, so an abort between entries is observed before the next forwardApply begins. Tests (+3, all revert-proof): endpoint forwards patch → engine merges (not stale overwrite); browser facade forwards patch; read-only writeFile rejects without calling updateCell; cancellation observed at loop boundary before the next entry starts. Also converts databaseModel.test.ts workspace.fs overrides to Object.defineProperty per convention. Deferred (own PR): hot-exit does not persist the redo/futureStack — pre-existing on main, affects desktop too. tsc clean; build OK; full suite 356/356. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 6 triage + fixes (commit 0a15b16)Three reviewers this round (CodeRabbit, Gemini, Codex connector). Several comments were re-anchored from earlier rounds; I verified each against current code. ✅ Already resolved in earlier rounds (verified, no change needed)
🔧 Fixed this round
⏭️ Deferred (own PR)
Verification (local, real env): tsc clean · build OK · full suite 356/356 (+3, all revert-proof: each fix's test fails when its source change is reverted). |
|
@coderabbitai review |
|
/gemini review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request enables read-write database editing in VS Code for Web by leveraging the in-process WASM engine. It introduces robust undo/redo history tracking, atomic modifications using savepoints, support for JSON merge patches, and prevents silent data loss on web save failures by keeping the edit history uncommitted if a write fails. Feedback on the changes suggests checking the cancellation token in the save method of DatabaseDocument to allow aborting the save operation gracefully.
| async save(cancellation?: vsc.CancellationToken): Promise<void> { | ||
| await this.ensureWritable(); |
There was a problem hiding this comment.
The cancellation token passed to the save method is currently ignored. For large databases or slow web filesystems, saving can be a time-consuming operation. Checking the cancellation token before performing the save and throwing a vsc.CancellationError allows VS Code to abort the operation gracefully if requested by the user.
| async save(cancellation?: vsc.CancellationToken): Promise<void> { | |
| await this.ensureWritable(); | |
| async save(cancellation?: vsc.CancellationToken): Promise<void> { | |
| if (cancellation?.isCancellationRequested) { | |
| throw new vsc.CancellationError(); | |
| } | |
| await this.ensureWritable(); |
…view round 7) Gemini review: save(cancellation?) accepted a CancellationToken but ignored it. The WASM web-save path (serialize in-memory db + workspace.fs.writeFile over a possibly-remote filesystem) can be slow, so a user-requested cancel should abort it gracefully rather than always running to completion. save() now throws vsc.CancellationError when the token is already cancelled — once on entry, and again before the WASM serialize/write begins (after the quick native-engine branch). The native branch returns fast and needs no mid-op check. Adds CancellationError to the vscode test mock and a revert-proof test: a pre-cancelled token makes save() reject with a Canceled error and perform no serialize/writeToFile work. tsc clean; build OK; full suite 357/357. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 7 (commit 1db571a)Only one new finding this round (Gemini, medium) — addressed.
All other open threads are either already-resolved (verified in earlier rounds) or the deferred pre-existing Verification (local): tsc clean · build OK · full suite 357/357 (+1, revert-proof). |
|
@coderabbitai review |
|
/gemini review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request enables read-write database editing in VS Code for Web by leveraging an in-process WASM engine. It introduces a robust hot-exit restore mechanism that replays uncommitted edit history into a fresh in-memory database on startup using SQLite SAVEPOINTs for atomicity. Key improvements include support for JSON merge patches during cell updates, prevention of silent data loss by surfacing save errors and keeping edit history uncommitted, and forcing read-only mode when active WAL files are detected in the browser. Additionally, the ModificationTracker was enhanced to track absolute timeline positions and handle checkpoint invalidation during asynchronous saves. I have no feedback to provide as there are no review comments.
|
The deferred hot-exit / |
…N merge, atomic recovery) Round 5-7 review hardening was user-facing but missing from the 1.5.0 notes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b67d8e0bf
ℹ️ 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".
| undoModification: (mod: ModificationEntry) => | ||
| endpoint.undoModification(mod), |
There was a problem hiding this comment.
When a browser cell edit is recorded as operation: 'json_patch', this newly wired undo path reaches WasmDatabaseEngine.undoCellUpdate, which ignores the operation and writes the stale priorValue as a full replacement. If another editor pane changed a different key after the webview captured originalValue, the live edit correctly merges the patch into the current JSON, but Undo then restores the old whole JSON document and drops the other pane's key. Please either store/replay an inverse patch for these entries or avoid patch-based history for edits that must be undone by value replacement.
Useful? React with 👍 / 👎.
1 participant
Summary
Enables editing SQLite databases in VS Code for Web (vscode.dev / github.dev). The web build was read-only since v1.0.0 — any edit reported "Document is read-only". That restriction was a conservative original choice from when the web engine path was unreliable; now that the in-process WASM engine works in the web extension host (1.3.9, verified), web editing is feasible.
Changes
editorController.ts— provider gate: drop the!VSCODE_BROWSER_EXTterm fromenableReadWrite, relying onSupportsWriteMode(now web-aware). The browser host gets the read-writeDatabaseEditorProvider(whoseisReadOnlyisfalse), sohostBridge.isReadOnlyno longer blocks edits. Desktop logic unchanged.databaseModel.ts—SupportsWriteMode = IsLocalMode || IsRemoteWorkspaceMode || IsBrowserExtensionHost.save()persists web edits byserializeDatabase()→workspace.fs.writeFile(uri, …)(the WASM/non-file://branch, which github.dev supports).Safety (the part that matters for data-writing)
createCheckpoint()(marks the undo tracker "saved") now runs only after a successful write, in every branch. Previously it ran before the write.Tests (+4, 338/338)
SupportsWriteModeallows the browser host.save()serializes +writeFiles for a non-file://uri.save: keeps failed non-file WASM writes uncommitted for backup and retry— the data-loss safeguard; revert-proof-verified (fails if the checkpoint reorder is undone).Unit tests prove the mechanism (provider selection, serialize+writeFile, failure-preservation), but not that a real edit round-trips on github.dev's filesystem. That requires publishing. Plan: publish 1.5.0, edit a cell in vscode.dev, save, reload, confirm the change persisted — before calling this done.
Build OK (
extension-browser.jsexportsactivate, worker-free) ·tsc --noEmitclean ·npm test338/338.Summary by CodeRabbit
New Features
Bug Fixes