chore: post-Jules-triage followups#338
Conversation
Tighten internal sql.js types in WasmDatabaseEngine to CellValue[], removing the `params as unknown[]` cast at the iterateStatements bind site (gap noted while closing #312). Export WorkerPort interface so #326's connectWorkerPort tests typecheck under `tsc --noEmit`, and apply matching variance/structural casts to rpc.test.ts and webviewMessageHandler.test.ts so the tightened types from #305 (`unknown[]`) and #330 (`HostBridge`) don't introduce new tsc errors. Switch the two remaining bare `import.meta.env.VSCODE_BROWSER_EXT` reads in editorController.ts to optional chaining, matching the pattern already used in threadPool.ts and cryptoShim.ts. This unblocks testing the read-only vs read-write registration branch. Add tests/unit/editorController.test.ts covering registerEditorProvider: - readOnly=true picks DatabaseViewerProvider regardless of verified - verified=false picks DatabaseViewerProvider - verified=true + readOnly=false picks DatabaseEditorProvider - retainContextWhenHidden=false is set - returns a disposable This closes the coverage gap from #292 that I closed without iterating. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR tightens type safety in the WASM database engine by replacing ChangesType Safety and Testing Improvements
Sequence Diagram(s)No sequence diagram generated; these changes are primarily type contract refinements, export additions, defensive guards, and test coverage rather than control-flow modifications. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 improves type safety in the WASM database engine by replacing unknown[] with CellValue[] and adds optional chaining to import.meta.env accesses to prevent crashes in environments where it is undefined. It also introduces a new unit test suite for the editorController and exports the WorkerPort interface. Feedback was provided regarding a change in rpc.test.ts where using unknown[] and type casting reduced type safety; a suggestion was made to restore the more specific function signature.
| const methods: Record<string, (...args: unknown[]) => unknown> = { | ||
| add: (...args: unknown[]) => (args[0] as number) + (args[1] as number) | ||
| }; |
There was a problem hiding this comment.
This change reduces type safety by using unknown[] and type casting. The original, more specific function signature (a: number, b: number) => a + b is preferable as it's more readable and safer, and it is assignable to the required (...args: unknown[]) => unknown type.
I suggest restoring the safer function implementation while keeping the explicit type annotation:
| const methods: Record<string, (...args: unknown[]) => unknown> = { | |
| add: (...args: unknown[]) => (args[0] as number) + (args[1] as number) | |
| }; | |
| const methods: Record<string, (...args: unknown[]) => unknown> = { | |
| add: (a: number, b: number) => a + b | |
| }; |
![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
🤖 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 34-46: The CellValue type used by the WasmPreparedStatement and
WasmDatabaseInstance interfaces is missing boolean support; update the CellValue
type definition to include boolean so binding boolean values is allowed (affects
usages in WasmPreparedStatement methods bind, run, get and WasmDatabaseInstance
methods exec and prepare), i.e. add boolean to the existing union for CellValue
so TypeScript reflects sql.js's accepted parameter types.
In `@tests/unit/editorController.test.ts`:
- Around line 10-14: The test currently assigns to readonly VS Code API fields
directly (ExtensionKind, env.remoteName, extensions) which is fragile; replace
those direct assignments by using Object.defineProperty on the mock object
(e.g., define ExtensionKind, define env with remoteName, and define
extensions.getExtension) so properties are set as non-writable/configurable as
needed; update the mocked symbols referenced in this file (ExtensionKind,
mockVscode.env.remoteName, and mockVscode.extensions.getExtension) to be defined
via Object.defineProperty to mirror VS Code readonly behavior.
🪄 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: ee31b1c2-9b39-40d5-bb66-6382caf26f46
📒 Files selected for processing (6)
src/core/engine/wasm/WasmDatabaseEngine.tssrc/core/rpc.tssrc/editorController.tstests/unit/editorController.test.tstests/unit/rpc.test.tstests/unit/webviewMessageHandler.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/rpc.test.tstests/unit/webviewMessageHandler.test.tstests/unit/editorController.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/rpc.tssrc/editorController.tssrc/core/engine/wasm/WasmDatabaseEngine.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/rpc.tssrc/editorController.tssrc/core/engine/wasm/WasmDatabaseEngine.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
🔇 Additional comments (7)
src/editorController.ts (1)
263-264: LGTM!Also applies to: 381-393
tests/unit/editorController.test.ts (1)
18-27: ⚡ Quick win
andsrc/core/engine/wasm/WasmDatabaseEngine.ts (2)
125-125: LGTM!
304-304: LGTM!src/core/rpc.ts (1)
348-351: LGTM!tests/unit/rpc.test.ts (1)
14-16: LGTM!tests/unit/webviewMessageHandler.test.ts (1)
16-16: LGTM!Also applies to: 54-54, 80-80
| interface WasmPreparedStatement { | ||
| run(params?: unknown[]): void; | ||
| bind(params?: unknown[]): boolean; | ||
| get(params?: unknown[]): CellValue[] | undefined; | ||
| run(params?: CellValue[]): void; | ||
| bind(params?: CellValue[]): boolean; | ||
| get(params?: CellValue[]): CellValue[] | undefined; | ||
| step(): boolean; | ||
| reset(): void; | ||
| free(): boolean; | ||
| getColumnNames(): string[]; | ||
| } | ||
|
|
||
| export interface WasmDatabaseInstance { | ||
| exec(sql: string, params?: unknown[]): Array<{ columns: string[]; values: unknown[][] }>; | ||
| prepare(sql: string, params?: unknown[]): WasmPreparedStatement; | ||
| exec(sql: string, params?: CellValue[]): Array<{ columns: string[]; values: CellValue[][] }>; | ||
| prepare(sql: string, params?: CellValue[]): WasmPreparedStatement; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check CellValue type definition to understand the constraint
rg -A 10 "type CellValue" --type=ts
# Search for sql.js usage patterns to see what types are actually passed
rg -C 3 "\.bind\(|\.run\(|\.exec\(" src/core/engine/wasm/ --type=tsRepository: zknpr/SQLite-Explorer
Length of output: 4829
🌐 Web query:
What parameter types does sql.js bind() method accept for prepared statement parameters?
💡 Result:
The bind method in sql.js accepts two primary parameter types to bind values to a prepared statement [1][2]. Depending on the type of parameters defined in your SQL statement, you should provide either an array or an object [1][2]: 1. Array: Use an array when binding values to anonymous (positional) parameters (e.g.,?,?NNN) [1][2]. The order of values in the array corresponds to the order of parameters in the SQL statement [2]. 2. Object: Use an object when binding values to named parameters (e.g.,:VVV, @VVV,
Citations:
- 1: https://sql.js.org/documentation/api.js.html
- 2: https://lovasoa.github.io/sql.js/documentation/class/Statement.html
- 3: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/sql.js/index.d.ts
Add boolean to the CellValue type definition to match sql.js parameter support.
The interface declarations constrain sql.js parameters to CellValue[] (string | number | null | Uint8Array), but sql.js's official documentation shows the bind() method accepts boolean values, which it maps to SQLite INTEGER. The CellValue type should include boolean to accurately reflect the library's capabilities and prevent type safety gaps when boolean data is used with prepared statements.
🤖 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/engine/wasm/WasmDatabaseEngine.ts` around lines 34 - 46, The
CellValue type used by the WasmPreparedStatement and WasmDatabaseInstance
interfaces is missing boolean support; update the CellValue type definition to
include boolean so binding boolean values is allowed (affects usages in
WasmPreparedStatement methods bind, run, get and WasmDatabaseInstance methods
exec and prepare), i.e. add boolean to the existing union for CellValue so
TypeScript reflects sql.js's accepted parameter types.
| (mockVscode as any).ExtensionKind = { Workspace: 2, UI: 1 }; | ||
| mockVscode.env.remoteName = 'remote'; | ||
| (mockVscode as any).extensions = { | ||
| getExtension: () => ({ extensionKind: 2 }) | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read its content
if [ -f "tests/unit/editorController.test.ts" ]; then
echo "=== File exists ==="
wc -l tests/unit/editorController.test.ts
echo ""
echo "=== First 30 lines (including imports and lines 10-14) ==="
head -n 30 tests/unit/editorController.test.ts
else
echo "File not found"
fiRepository: zknpr/SQLite-Explorer
Length of output: 1441
🏁 Script executed:
# Also check for vscode_mock_setup import
rg -n "vscode_mock_setup" tests/unit/editorController.test.ts | head -5Repository: zknpr/SQLite-Explorer
Length of output: 97
🏁 Script executed:
# Search for any other uses of ExtensionKind, env.remoteName, or extensions in the test file
rg -n "(ExtensionKind|remoteName|extensions)" tests/unit/editorController.test.tsRepository: zknpr/SQLite-Explorer
Length of output: 289
Use Object.defineProperty for mocked VS Code readonly fields.
Direct assignment to VS Code API fields like ExtensionKind, env.remoteName, and extensions should use Object.defineProperty instead for better test robustness and consistency with readonly property expectations.
Suggested patch
-(mockVscode as any).ExtensionKind = { Workspace: 2, UI: 1 };
-mockVscode.env.remoteName = 'remote';
-(mockVscode as any).extensions = {
- getExtension: () => ({ extensionKind: 2 })
-};
+Object.defineProperty(mockVscode as object, 'ExtensionKind', {
+ value: { Workspace: 2, UI: 1 },
+ writable: true,
+ configurable: true,
+});
+Object.defineProperty(mockVscode.env as object, 'remoteName', {
+ value: 'remote',
+ writable: true,
+ configurable: true,
+});
+Object.defineProperty(mockVscode as object, 'extensions', {
+ value: { getExtension: () => ({ extensionKind: 2 }) },
+ writable: true,
+ configurable: true,
+});📝 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.
| (mockVscode as any).ExtensionKind = { Workspace: 2, UI: 1 }; | |
| mockVscode.env.remoteName = 'remote'; | |
| (mockVscode as any).extensions = { | |
| getExtension: () => ({ extensionKind: 2 }) | |
| }; | |
| Object.defineProperty(mockVscode as object, 'ExtensionKind', { | |
| value: { Workspace: 2, UI: 1 }, | |
| writable: true, | |
| configurable: true, | |
| }); | |
| Object.defineProperty(mockVscode.env as object, 'remoteName', { | |
| value: 'remote', | |
| writable: true, | |
| configurable: true, | |
| }); | |
| Object.defineProperty(mockVscode as object, 'extensions', { | |
| value: { getExtension: () => ({ extensionKind: 2 }) }, | |
| writable: true, | |
| configurable: true, | |
| }); |
🤖 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/editorController.test.ts` around lines 10 - 14, The test currently
assigns to readonly VS Code API fields directly (ExtensionKind, env.remoteName,
extensions) which is fragile; replace those direct assignments by using
Object.defineProperty on the mock object (e.g., define ExtensionKind, define env
with remoteName, and define extensions.getExtension) so properties are set as
non-writable/configurable as needed; update the mocked symbols referenced in
this file (ExtensionKind, mockVscode.env.remoteName, and
mockVscode.extensions.getExtension) to be defined via Object.defineProperty to
mirror VS Code readonly behavior.
Apply review feedback from PR #338: line 39 read `mockVscode.window.registerCustomEditorProvider` without the `as any` cast that line 40 has — the mock window object doesn't declare this property, so it introduced a fresh TS2339, regressing the tsc-clean invariant that this PR was meant to restore. Add the matching cast. Also drop `before, after` from the import list since the test only uses `beforeEach`/`afterEach`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
Three coverage/typing followups noted while triaging the Jules + dependabot PR backlog.
WasmDatabaseEngine: tighten internal sql.js types
The internal
WasmPreparedStatement/WasmDatabaseInstanceinterfaces usedunknown[]for bind/run params, forcing aparams as unknown[]cast at theiterateStatementssite. Switched toCellValue[]everywhere internally, dropping the cast. This is the fix #312 should have made (#312 choseunknown[]which would now be even further from the mergedSqlValue[]declarations in #306).Export
WorkerPort#326 (connectWorkerPort tests) imported
WorkerPortwhich wasn't exported fromsrc/core/rpc.ts— tsx tolerated it, buttsc --noEmitflagged TS2459. Addingexportkeyword; no behavior change.Test type fixes downstream of #305 and #330
#305 tightened
MethodImplementationsto(...args: unknown[]) => unknown(fromany[]); #330 tightenedWebviewMessageHandler's hostBridge param to the realHostBridgetype. Both correctly tightened production code but the existing tests passed widened/partial mocks that no longer satisfy the stricter types. Cast at the test call sites to maketsc --noEmitclean again without weakening the production signatures.registerEditorProvidertest (closes #292 gap)Switched the two remaining bare
import.meta.env.VSCODE_BROWSER_EXTreads ineditorController.tsto optional chaining (consistent withthreadPool.tsandcryptoShim.tswhich already do this), so the module can be required fromtsxwithout throwing. Addedtests/unit/editorController.test.tscovering:readOnly=true→DatabaseViewerProvider(regardless ofverified)verified=false→DatabaseViewerProviderverified=true+readOnly=false→DatabaseEditorProviderretainContextWhenHidden=falsein webview optionsThis closes the coverage gap from #292 (which was closed without iterating).
Test plan
npm test→ 269 pass, 0 fail (was 264; +5 from new file)npx tsc --noEmiton src/ → 0 errors (baseline had 1 pre-existing in hostBridge.ts that the ⚡ [Performance] Eliminate concurrent IPC map-promise-all in hostBridge cell batch updates #323 merge resolved as a side effect)registerEditorProvidersemantics — optional-chain replacement is a no-op whenimport.meta.envis the defined object, and graceful when it's undefined (test path)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Refactor