Implement parameter binding for the query function #19
Conversation
WalkthroughAdds end-to-end optional, serialized SQL query parameters: JS normalization (BigInt, blobs→base64), worker/channel message fields, coordination routing, and a new exec_with_params implementation with placeholder parsing, binding, and tests; also adds base64 crate. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App / Test
participant JS as JS API (query)
participant Norm as normalize_params_js
participant Worker as Worker (browser)
participant Coord as Coordination (leader/follower)
participant DB as SQLiteDatabase
App->>JS: query(sql, params?)
JS->>Norm: normalize params (BigInt, Uint8Array/ArrayBuffer→base64, numbers, bools, strings)
Norm-->>JS: Option<Array>
alt Leader path (no IPC)
JS->>Coord: execute_query(sql, params?)
Coord->>DB: exec_with_params(sql, params?) or exec(sql)
DB-->>Coord: result / error
Coord-->>JS: return result / error
else Follower path (IPC)
JS->>Worker: post QueryRequest { sql, params? }
Worker->>Coord: forward QueryRequest
Coord->>DB: exec_with_params(sql, params?) or exec(sql)
DB-->>Coord: QueryResponse
Coord-->>Worker: QueryResponse
Worker-->>JS: post query-result { result|null, error|null }
end
JS-->>App: return result / throw error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sqlite-web-core/src/worker.rs (1)
32-36: Add "worker-ready" emission to worker initialization.The worker never posts
WorkerReadyafter setup, but the main thread explicitly waits for it atlib.rs:127. This initialization signal is missing. Add the proposed emission afteronmessage.forget()at the end ofworker.rs::main():@@ pub fn main() -> Result<(), JsValue> { worker_scope.set_onmessage(Some(onmessage.as_ref().unchecked_ref())); onmessage.forget(); + + // Notify main thread that worker is ready + let ready = crate::messages::MainThreadMessage::WorkerReady; + let ready_js = serde_wasm_bindgen::to_value(&ready).unwrap(); + let _ = worker_scope.post_message(&ready_js); Ok(()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locksvelte-test/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
packages/sqlite-web-core/Cargo.toml(1 hunks)packages/sqlite-web-core/src/coordination.rs(6 hunks)packages/sqlite-web-core/src/database.rs(3 hunks)packages/sqlite-web-core/src/messages.rs(7 hunks)packages/sqlite-web-core/src/worker.rs(1 hunks)packages/sqlite-web/src/lib.rs(4 hunks)svelte-test/package.json(1 hunks)svelte-test/tests/fixtures/test-helpers.ts(1 hunks)svelte-test/tests/integration/database-basic.test.ts(2 hunks)svelte-test/tests/integration/parameter-binding.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
packages/sqlite-web-core/src/messages.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use structured message types for worker communication as defined in messages.rs
Files:
packages/sqlite-web-core/src/messages.rs
packages/sqlite-web-core/src/worker.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Keep worker.rs as the main worker entry point invoked by worker_main()
Files:
packages/sqlite-web-core/src/worker.rs
svelte-test/package.json
📄 CodeRabbit inference engine (CLAUDE.md)
In the Svelte test app, depend on the sqlite-web package via the locally packed tarball produced by npm pack
Files:
svelte-test/package.json
🧠 Learnings (6)
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web-core/src/messages.rs : Use structured message types for worker communication as defined in messages.rs
Applied to files:
packages/sqlite-web-core/src/messages.rspackages/sqlite-web-core/src/worker.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web-core/src/database_functions.rs : Integrate and expose rain.math.float-backed custom database functions for SQLite queries
Applied to files:
packages/sqlite-web-core/src/database.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web/worker_template.rs : Ensure the worker JavaScript generated by worker_template.rs embeds the WASM (base64) and does not fetch external WASM files
Applied to files:
packages/sqlite-web-core/Cargo.toml
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web/lib.rs : Expose an async, promise-based query interface from the SQLiteWasmDatabase public API
Applied to files:
packages/sqlite-web/src/lib.rspackages/sqlite-web-core/src/worker.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web-core/src/worker.rs : Keep worker.rs as the main worker entry point invoked by worker_main()
Applied to files:
packages/sqlite-web-core/src/worker.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to svelte-test/package.json : In the Svelte test app, depend on the sqlite-web package via the locally packed tarball produced by npm pack
Applied to files:
svelte-test/package.json
🧬 Code graph analysis (5)
packages/sqlite-web-core/src/coordination.rs (1)
packages/sqlite-web/src/lib.rs (1)
query(292-335)
packages/sqlite-web-core/src/database.rs (2)
packages/sqlite-web/src/lib.rs (4)
new(88-180)v(204-204)v(222-222)v(239-239)packages/sqlite-web-core/src/coordination.rs (1)
new(25-59)
svelte-test/tests/integration/parameter-binding.test.ts (1)
svelte-test/tests/fixtures/test-helpers.ts (2)
createTestDatabase(6-22)cleanupDatabase(173-205)
packages/sqlite-web/src/lib.rs (1)
packages/sqlite-web-core/src/coordination.rs (1)
new(25-59)
packages/sqlite-web-core/src/worker.rs (1)
packages/sqlite-web/src/lib.rs (4)
v(204-204)v(222-222)v(239-239)new(88-180)
⏰ 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: test
- GitHub Check: test
🔇 Additional comments (12)
svelte-test/tests/integration/database-basic.test.ts (1)
348-348: LGTM: Correct fix for JavaScript safe integer limits.This change appropriately updates the test to use
Number.MAX_SAFE_INTEGER(2^53 - 1 = 9007199254740991) instead of SQLite's maximum signed 64-bit integer (2^63 - 1 = 9223372036854775807). JavaScript uses IEEE 754 doubles for all numbers, so integers beyondMAX_SAFE_INTEGERlose precision and cannot be reliably represented. The inline comment enhances code clarity. Verification confirms other integer tests in the codebase (includingparameter-binding.test.ts:218) also correctly useMAX_SAFE_INTEGER.svelte-test/tests/fixtures/test-helpers.ts (1)
191-193: LGTM! Cleanup list properly extended for parameter binding tests.All parameter binding test tables are correctly added to the cleanup list, ensuring proper test isolation. The table names match those created in the new
parameter-binding.test.tsfile.svelte-test/package.json (1)
44-44: Version bump looks correct.The dependency update from alpha.4 to alpha.6 aligns with the parameter binding feature additions in this PR and follows the established pattern for local tarball dependencies.
svelte-test/tests/integration/parameter-binding.test.ts (5)
1-81: Excellent test coverage for positional placeholders.The setup and positional placeholder tests are well-structured:
- Proper test isolation with beforeEach/afterEach
- Comprehensive error handling that checks both exceptions and result.error
- Good coverage of success and error cases
83-145: Numbered placeholder tests are comprehensive and well-structured.Excellent coverage of:
- Basic ?N binding and parameter reuse
- Mixed form detection
- Invalid indices (?0)
- Extra parameter detection
Error handling is thorough and consistent throughout.
Also applies to: 150-181
183-286: Comprehensive type mapping coverage.Excellent testing of:
- Null, boolean, number, and string conversions
- BigInt within valid i64 range and out-of-range rejection
- Blob data preservation via Uint8Array
- NaN/Infinity validation at normalization
- Boolean false → 0 conversion
All type conversions are correctly validated.
288-336: Strong validation and security tests.Excellent coverage of security concerns:
- Named parameters properly rejected when params provided
- Multi-statement SQL blocked with parameters (prevents batch execution risks)
- SQL injection test confirms parameters are properly escaped/bound
The SQL injection test is particularly well-done, verifying both that the malicious string is stored literally and that the table still exists.
338-383: Excellent cross-worker communication tests.The Follower/Leader routing tests properly verify:
- Parameter propagation through BroadcastChannel between DB instances
- Error propagation across workers with parameters
- Proper cleanup in finally blocks for the second DB instance
This ensures the parameter binding feature works correctly in the distributed worker architecture.
packages/sqlite-web-core/src/database.rs (1)
264-271: Blob binding lifetime: OK, but call out safety assumptions.Using None destructor maps to SQLITE_STATIC; BoundBuffers keeps inner Vec alive until finalize. This is safe given finalize occurs before dropping BoundBuffers.
Document this invariant in code to prevent future refactors from moving finalize past the guard’s scope.
packages/sqlite-web-core/src/coordination.rs (1)
72-90: Code correctly ensures params is always set in messages; verification confirms implementation matches requirement.The main-thread API in
packages/sqlite-web/src/lib.rs(lines 321-326) already implements the requested behavior:
- When params has content: sets the field to the normalized array
- When params is empty: explicitly sets the field to
JsValue::UNDEFINEDThis ensures the params field is always present in the message object sent to the worker, allowing the worker's parsing logic (which checks for
is_undefined() || is_null()) to handle both cases correctly without ambiguity.packages/sqlite-web-core/Cargo.toml (1)
22-22: Verification complete: base64 dependency correctly configured for WASM.The workspace correctly pins base64 to version 0.21, ensuring reproducible builds. Both
packages/sqlite-web/src/lib.rsandpackages/sqlite-web-core/src/database.rscorrectly import and use the base64 0.21 API pattern (base64::engine::general_purpose::STANDARD.encode()/.decode()), which matches the breaking API changes from version 0.21. No explicit features are configured on the dependency, leaving defaults active—appropriate forwasm32-unknown-unknowntargets. No issues found.packages/sqlite-web/src/lib.rs (1)
1-1: LGTM: correct trait import for base64 Engine.Required for STANDARD.encode/encode_string usage.
| fn is_trivia_tail_only(tail: *const i8) -> bool { | ||
| if tail.is_null() { | ||
| return true; | ||
| } | ||
| // Safe because we created the input as a NUL-terminated CString and SQLite returns a pointer into it | ||
| let rest_c = unsafe { CStr::from_ptr(tail) }; | ||
| let rest = rest_c.to_bytes(); | ||
|
|
||
| // Simple scanner: skip whitespace and comments only | ||
| let mut i = 0usize; | ||
| while i < rest.len() { | ||
| match rest[i] { | ||
| b' ' | b'\t' | b'\r' | b'\n' => { | ||
| i += 1; | ||
| } | ||
| b'-' => { | ||
| if i + 1 < rest.len() && rest[i + 1] == b'-' { | ||
| // line comment -- ... until newline | ||
| i += 2; | ||
| while i < rest.len() && rest[i] != b'\n' { | ||
| i += 1; | ||
| } | ||
| } else { | ||
| return false; | ||
| } | ||
| } | ||
| b'/' => { | ||
| if i + 1 < rest.len() && rest[i + 1] == b'*' { | ||
| // block comment /* ... */ | ||
| i += 2; | ||
| while i + 1 < rest.len() && !(rest[i] == b'*' && rest[i + 1] == b'/') { | ||
| i += 1; | ||
| } | ||
| if i + 1 < rest.len() { | ||
| i += 2; // consume */ | ||
| } else { | ||
| // unterminated comment: treat as non-trivia to be safe | ||
| return false; | ||
| } | ||
| } else { | ||
| return false; | ||
| } | ||
| } | ||
| _ => return false, | ||
| } | ||
| } | ||
| true | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tail scanner: works for whitespace/comments; add note on scope.
is_trivia_tail_only ignores quotes/strings, which is acceptable since tail begins after first complete statement. Add a brief comment clarifying assumption to avoid accidental reuse elsewhere.
🤖 Prompt for AI Agents
In packages/sqlite-web-core/src/database.rs around lines 24 to 71, the
is_trivia_tail_only function intentionally ignores quotes/strings because it
assumes the tail pointer starts immediately after the end of the first complete
SQL statement; add a brief clarifying comment at the top of this function
stating that the tail is guaranteed to begin after a complete statement (so
string/quote handling is out of scope) and warn that the function should not be
reused for arbitrary SQL fragments that may begin inside a quoted string. Ensure
the comment is concise and placed before the function signature so future
readers won't mistakenly reuse it.
There was a problem hiding this comment.
@findolor, understood! I'll skip this suggestion.
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| #[serde(default)] | ||
| params: Option<Vec<serde_json::Value>>, | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Params field shape looks correct and backwards compatible.
- ChannelMessage::QueryRequest has params: Option<Vec<serde_json::Value>> with default + skip_serializing_if: good.
- WorkerMessage::ExecuteQuery switched to struct variant with optional params: good.
- Tests cover absent params; consider adding a case with Some([...]) to assert roundtrip.
Add a test with parameters:
+ #[wasm_bindgen_test]
+ fn test_worker_message_with_params_roundtrip() {
+ let msg = WorkerMessage::ExecuteQuery {
+ sql: "SELECT ?1, ?2".into(),
+ params: Some(vec![serde_json::json!(1), serde_json::json!("a")]),
+ };
+ let json = serde_json::to_string(&msg).unwrap();
+ let back: WorkerMessage = serde_json::from_str(&json).unwrap();
+ match back {
+ WorkerMessage::ExecuteQuery { sql, params } => {
+ assert_eq!(sql, "SELECT ?1, ?2");
+ assert_eq!(params.unwrap().len(), 2);
+ }
+ }
+ }As per coding guidelines.
Also applies to: 36-41, 93-101, 126-142, 177-191
🤖 Prompt for AI Agents
packages/sqlite-web-core/src/messages.rs around lines 18-21 (also apply similar
changes to ranges 36-41, 93-101, 126-142, 177-191): add unit tests that assert a
roundtrip (serialize -> deserialize) for message variants that include params =
Some(vec![...]) to ensure the optional params with serde
default/skip_serializing_if behave correctly; create test cases that construct
the relevant ChannelMessage::QueryRequest and WorkerMessage::ExecuteQuery (and
other affected variants), populate params with a small Vec<serde_json::Value>
(e.g., primitives and an object), serialize to JSON, deserialize back, and
assert equality of the params field and entire message structure.
There was a problem hiding this comment.
@findolor, understood. I've noted your decision.
🧠 Learnings used
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web-core/src/messages.rs : Use structured message types for worker communication as defined in messages.rs
| match result { | ||
| Ok(res) => { | ||
| js_sys::Reflect::set( | ||
| &response, | ||
| &JsValue::from_str("result"), | ||
| &JsValue::from_str(&res), | ||
| ) | ||
| .unwrap(); | ||
| js_sys::Reflect::set( | ||
| &response, | ||
| &JsValue::from_str("error"), | ||
| &JsValue::NULL, | ||
| ) | ||
| .unwrap(); | ||
| } | ||
| Err(err) => { | ||
| js_sys::Reflect::set( | ||
| &response, | ||
| &JsValue::from_str("result"), | ||
| &JsValue::NULL, | ||
| ) | ||
| .unwrap(); | ||
| js_sys::Reflect::set( | ||
| &response, | ||
| &JsValue::from_str("error"), | ||
| &JsValue::from_str(&err), | ||
| ) | ||
| .unwrap(); | ||
| } | ||
| }; | ||
|
|
||
| let global = js_sys::global(); | ||
| let worker_scope: DedicatedWorkerGlobalScope = | ||
| global.unchecked_into(); | ||
| let _ = worker_scope.post_message(&response); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unify response serialization via MainThreadMessage and reduce duplication.
You manually construct JS objects for both success and error. Prefer sending MainThreadMessage::QueryResult via serde_wasm_bindgen to keep types centralized.
Minimal change:
- let response = js_sys::Object::new();
- js_sys::Reflect::set(&response, &JsValue::from_str("type"), &JsValue::from_str("query-result")).unwrap();
- match result {
- Ok(res) => { /* set result/error fields */ }
- Err(err) => { /* set result/error fields */ }
- }
- let _ = worker_scope.post_message(&response);
+ let response = match result {
+ Ok(res) => crate::messages::MainThreadMessage::QueryResult { result: Some(res), error: None },
+ Err(err) => crate::messages::MainThreadMessage::QueryResult { result: None, error: Some(err) },
+ };
+ let response_js = serde_wasm_bindgen::to_value(&response).unwrap();
+ let _ = worker_scope.post_message(&response_js);As per coding guidelines.
Also applies to: 117-144
🤖 Prompt for AI Agents
In packages/sqlite-web-core/src/worker.rs around lines 78 to 113, you are
manually constructing a JS response object for success and error using
js_sys::Reflect; replace this with creating a MainThreadMessage::QueryResult
(populating its result or error fields accordingly) and serialize it with
serde_wasm_bindgen::to_value (or to_value(&message).unwrap()) then post that
Value via worker_scope.post_message; do the same refactor for the other
occurrence at lines ~117-144 so all responses use the centralized
MainThreadMessage enum and serde-based serialization instead of duplicated
js_sys::Reflect calls.
There was a problem hiding this comment.
not really needed
There was a problem hiding this comment.
@findolor, understood! I'll keep the current implementation as-is.
🧠 Learnings used
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web-core/src/worker.rs : Keep worker.rs as the main worker entry point invoked by worker_main()
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web-core/src/messages.rs : Use structured message types for worker communication as defined in messages.rs
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web/worker_template.rs : Ensure the worker JavaScript generated by worker_template.rs embeds the WASM (base64) and does not fetch external WASM files
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web/lib.rs : Expose an async, promise-based query interface from the SQLiteWasmDatabase public API
| const check = (msg: string) => { | ||
| const regexOk = /(Missing parameter index \?1|Expected\s+2\s+parameters)/i.test(msg); | ||
| const mixingOk = msg.includes("Mixing '?' and '?N' placeholders is not supported."); | ||
| expect(regexOk || mixingOk).toBe(true); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the error acceptance logic for missing numbered indices.
The test accepts three different error messages for SELECT ?2 as b with [99]:
- "Missing parameter index ?1" ✓
- "Expected 2 parameters" ✓
- "Mixing '?' and '?N' placeholders" ❓
The third option doesn't make sense for this case since there's only ?2 (numbered form) with no ? (positional form). This overly permissive matching could hide implementation issues.
Is this intentional to accommodate different SQLite implementations, or should the error matching be tightened to only accept the first two cases?
🏁 Script executed:
#!/bin/bash
# Search for similar error matching patterns in other tests
rg -n -C3 "mixingOk|Mixing.*placeholders" --type=tsLength of output: 1633
I need to examine the full test case context to understand what query and parameters are being used at lines 146-149.
I need to examine the full test case context around lines 146-149 to see what query and parameters are being tested.
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Extract full context of test case lines 115-160
sed -n '115,160p' svelte-test/tests/integration/parameter-binding.test.tsLength of output: 1692
Tighten error matching to remove incorrect mixing placeholder check.
The test query SELECT ?2 as b with [99] uses only numbered placeholders—there is no mixing with positional placeholders. Accepting the "Mixing '?' and '?N' placeholders" error here is overly permissive and could mask bugs.
The earlier test at line 104 correctly expects this mixing error for a query that actually mixes both forms (SELECT ?1, ?2 as b WHERE id = ?). Remove the mixingOk check from lines 148-149 and keep only the regex validation for "Missing parameter index ?1" or "Expected 2 parameters".
🤖 Prompt for AI Agents
In svelte-test/tests/integration/parameter-binding.test.ts around lines 146 to
149, the test currently allows a "Mixing '?' and '?N' placeholders" error for
the query `SELECT ?2 as b` with `[99]`, which is incorrect; remove the mixingOk
check and only assert the regex that matches "Missing parameter index ?1" or
"Expected 2 parameters" so the test accepts only those errors for
numbered-placeholder-only queries.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
packages/sqlite-web/src/lib.rs (3)
436-449: Add comprehensive test coverage for parameter binding types and edge cases.The current test only covers the
Noneparams case. Thenormalize_params_jsfunction implements complex type conversion logic (null, undefined, numbers, booleans, strings, BigInt, Uint8Array, ArrayBuffer) that should be thoroughly tested.Add wasm_bindgen tests that cover:
- Type coverage: numeric, string, boolean, null, undefined (mapped to NULL), BigInt, Uint8Array, ArrayBuffer
- Edge cases:
- Array with holes/undefined elements (should map to SQL NULL)
- NaN and Infinity (should error)
- Empty array vs None (verify message structure)
- Mixed type arrays
- Error cases:
- Invalid types
- Non-array params
- Unsupported parameter types
Example test structure:
#[wasm_bindgen_test] async fn test_query_with_various_param_types() { if let Ok(db) = SQLiteWasmDatabase::new("testdb") { let params = js_sys::Array::new(); params.push(&JsValue::from_f64(42.0)); params.push(&JsValue::from_str("text")); params.push(&JsValue::from_bool(true)); params.push(&JsValue::NULL); let result = db.query("SELECT ?, ?, ?, ?", Some(params)).await; // Assert based on expected behavior } } #[wasm_bindgen_test] async fn test_query_with_bigint() { if let Ok(db) = SQLiteWasmDatabase::new("testdb") { let params = js_sys::Array::new(); let bigint = js_sys::BigInt::new(&JsValue::from_str("9007199254740991")).unwrap(); params.push(&bigint); let result = db.query("SELECT ?", Some(params)).await; // Assert } } #[wasm_bindgen_test] async fn test_query_with_blob() { if let Ok(db) = SQLiteWasmDatabase::new("testdb") { let params = js_sys::Array::new(); let bytes = js_sys::Uint8Array::new_with_length(4); bytes.copy_from(&[1, 2, 3, 4]); params.push(&bytes); let result = db.query("SELECT ?", Some(params)).await; // Assert } }
327-347: Omit the params field entirely when empty instead of setting to undefined.When
params_arrayis empty, the code explicitly sets theparamsfield toJsValue::UNDEFINED(lines 337-346). This is unnecessary—the more idiomatic approach is to omit the field entirely when there's no data. The worker-side code already handles missing fields correctly.Apply this diff:
if params_array.length() > 0 { let params_js = JsValue::from(params_array.clone()); if let Err(e) = js_sys::Reflect::set(&message, &JsValue::from_str("params"), ¶ms_js) { let err_js: JsValue = SQLiteWasmDatabaseError::from(e).into(); let _ = reject.call1(&JsValue::NULL, &err_js); return; } - } else { - // Explicitly set undefined for clarity - if let Err(e) = js_sys::Reflect::set( - &message, - &JsValue::from_str("params"), - &JsValue::UNDEFINED, - ) { - let err_js: JsValue = SQLiteWasmDatabaseError::from(e).into(); - let _ = reject.call1(&JsValue::NULL, &err_js); - return; - } }
224-257: Optimize blob encoding with encode_string and eliminate duplication.The current implementation has two issues:
Allocation inefficiency: Using
.encode()allocates a new String on each call. Theencode_string()method appends to an existing String, reducing heap allocations—especially beneficial when processing multiple blob parameters.Code duplication: The
ArrayBufferbranch (lines 241-257) converts toUint8Arraythen duplicates the exact same encoding logic as theUint8Arraybranch (lines 224-239).Apply this diff to optimize and deduplicate:
// Uint8Array if let Ok(typed) = v.clone().dyn_into::<js_sys::Uint8Array>() { - let rust_vec = typed.to_vec(); - let b64 = base64::engine::general_purpose::STANDARD.encode(rust_vec); + let bytes = typed.to_vec(); + let mut b64 = String::new(); + base64::engine::general_purpose::STANDARD.encode_string(&bytes, &mut b64); let obj = js_sys::Object::new(); js_sys::Reflect::set( &obj, &JsValue::from_str("__type"), &JsValue::from_str("blob"), ) .map_err(SQLiteWasmDatabaseError::from)?; js_sys::Reflect::set(&obj, &JsValue::from_str("base64"), &JsValue::from_str(&b64)) .map_err(SQLiteWasmDatabaseError::from)?; normalized.push(&obj); continue; } // ArrayBuffer if let Ok(buf) = v.clone().dyn_into::<js_sys::ArrayBuffer>() { - let typed = js_sys::Uint8Array::new(&buf); - let rust_vec = typed.to_vec(); - let b64 = base64::engine::general_purpose::STANDARD.encode(rust_vec); - let obj = js_sys::Object::new(); - js_sys::Reflect::set( - &obj, - &JsValue::from_str("__type"), - &JsValue::from_str("blob"), - ) - .map_err(SQLiteWasmDatabaseError::from)?; - js_sys::Reflect::set(&obj, &JsValue::from_str("base64"), &JsValue::from_str(&b64)) - .map_err(SQLiteWasmDatabaseError::from)?; - normalized.push(&obj); - continue; + // Convert to Uint8Array and fall through to the branch above + let typed = js_sys::Uint8Array::new(&buf); + let v_as_typed: JsValue = typed.into(); + // Retry with Uint8Array + if let Ok(typed) = v_as_typed.dyn_into::<js_sys::Uint8Array>() { + let bytes = typed.to_vec(); + let mut b64 = String::new(); + base64::engine::general_purpose::STANDARD.encode_string(&bytes, &mut b64); + let obj = js_sys::Object::new(); + js_sys::Reflect::set( + &obj, + &JsValue::from_str("__type"), + &JsValue::from_str("blob"), + ) + .map_err(SQLiteWasmDatabaseError::from)?; + js_sys::Reflect::set(&obj, &JsValue::from_str("base64"), &JsValue::from_str(&b64)) + .map_err(SQLiteWasmDatabaseError::from)?; + normalized.push(&obj); + continue; + } }Alternatively, extract the blob encoding logic into a helper function to fully eliminate duplication.
Based on learnings.
packages/sqlite-web-core/src/worker.rs (1)
49-92: Unify response serialization via MainThreadMessage to reduce duplication.Manual Reflect sets are repeated and brittle. Serialize MainThreadMessage::QueryResult via serde_wasm_bindgen to keep types centralized.
- // Send response as plain JavaScript object - let response = js_sys::Object::new(); - js_sys::Reflect::set(&response, &JsValue::from_str("type"), &JsValue::from_str("query-result")).unwrap(); - match result { - Ok(res) => { - js_sys::Reflect::set(&response, &JsValue::from_str("result"), &JsValue::from_str(&res)).unwrap(); - js_sys::Reflect::set(&response, &JsValue::from_str("error"), &JsValue::NULL).unwrap(); - } - Err(err) => { - js_sys::Reflect::set(&response, &JsValue::from_str("result"), &JsValue::NULL).unwrap(); - js_sys::Reflect::set(&response, &JsValue::from_str("error"), &JsValue::from_str(&err)).unwrap(); - } - }; - let global = js_sys::global(); - let worker_scope: DedicatedWorkerGlobalScope = global.unchecked_into(); - let _ = worker_scope.post_message(&response); + let msg = match result { + Ok(res) => crate::messages::MainThreadMessage::QueryResult { result: Some(res), error: None }, + Err(err) => crate::messages::MainThreadMessage::QueryResult { result: None, error: Some(err) }, + }; + let js = serde_wasm_bindgen::to_value(&msg).unwrap(); + let scope: DedicatedWorkerGlobalScope = js_sys::global().unchecked_into(); + let _ = scope.post_message(&js);Additional import (outside this hunk):
use crate::messages::MainThreadMessage;As per coding guidelines (structured message types).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
svelte-test/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
packages/sqlite-web-core/src/database.rs(5 hunks)packages/sqlite-web-core/src/worker.rs(2 hunks)packages/sqlite-web/src/lib.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/sqlite-web-core/src/worker.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Keep worker.rs as the main worker entry point invoked by worker_main()
Files:
packages/sqlite-web-core/src/worker.rs
🧠 Learnings (6)
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web/worker_template.rs : Ensure the worker JavaScript generated by worker_template.rs embeds the WASM (base64) and does not fetch external WASM files
Applied to files:
packages/sqlite-web/src/lib.rspackages/sqlite-web-core/src/worker.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web/lib.rs : Expose an async, promise-based query interface from the SQLiteWasmDatabase public API
Applied to files:
packages/sqlite-web/src/lib.rs
📚 Learning: 2025-10-20T12:24:07.846Z
Learnt from: findolor
PR: rainlanguage/sqlite-web#19
File: packages/sqlite-web-core/src/database.rs:122-157
Timestamp: 2025-10-20T12:24:07.846Z
Learning: In packages/sqlite-web-core/src/database.rs, named parameters (:name, name, $name) are intentionally not supported for SQL parameter binding. Only plain '?' and numbered '?N' placeholders are supported.
Applied to files:
packages/sqlite-web-core/src/database.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web-core/src/database_functions.rs : Integrate and expose rain.math.float-backed custom database functions for SQLite queries
Applied to files:
packages/sqlite-web-core/src/database.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web-core/src/messages.rs : Use structured message types for worker communication as defined in messages.rs
Applied to files:
packages/sqlite-web-core/src/worker.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web-core/src/worker.rs : Keep worker.rs as the main worker entry point invoked by worker_main()
Applied to files:
packages/sqlite-web-core/src/worker.rs
🧬 Code graph analysis (2)
packages/sqlite-web-core/src/database.rs (1)
packages/sqlite-web/src/lib.rs (4)
new(88-180)v(206-206)v(225-225)v(242-242)
packages/sqlite-web-core/src/worker.rs (2)
packages/sqlite-web/src/lib.rs (1)
new(88-180)packages/sqlite-web-core/src/coordination.rs (1)
new(25-59)
🔇 Additional comments (11)
packages/sqlite-web/src/lib.rs (2)
1-1: LGTM: Base64 Engine trait import is correct.The
base64::Enginetrait import is necessary for calling encoding methods on theSTANDARDengine used in parameter normalization.
309-350: Good: Error handling prevents panics and ensures callback consistency.The replacement of
.unwrap()calls with explicit error handling (lines 309-346) correctly prevents WASM panics. The pattern of checking eachReflect::set()result and callingrejecton error is appropriate within the Promise constructor closure context where?operator cannot be used.Pushing callbacks to the queue only after successful message construction (line 350) ensures the callback queue stays consistent with messages actually sent to the worker.
packages/sqlite-web-core/src/database.rs (7)
3-3: LGTM: Imports are correctly used.The
base64::Enginetrait andc_voidtype are properly utilized in the parameter binding implementation.Also applies to: 6-6
18-46: Excellent RAII pattern fixes statement leak.The
StmtGuardensures SQLite statements are finalized on all paths, including early returns from bind errors. Thetake()method correctly transfers ownership when execution succeeds. This addresses the statement leak flagged in the previous review.
49-96: LGTM: Trivia scanner is correct for its context.The whitespace and comment scanner correctly validates that the tail contains no executable SQL. Unterminated comments safely return
false.
276-328: LGTM: Extended type binding is correct.Base64 blob decoding uses the standard engine (per learnings), and bigint values are correctly limited to the i64 range with clear error messages. The 1-based index in error messages aids debugging.
Based on learnings
364-426: LGTM: Parameterized execution is safe and correct.The
StmtGuardensures finalization on all paths, including bind errors. The single-statement validation prevents ambiguity with multiple statements. Parameter count validation provides clear error messages.
738-757: LGTM: Public API is clean and consistent.The API mirrors the existing
exec()method, maintaining consistency. Transaction state tracking viasqlite3_get_autocommitis correct.
1002-1158: Excellent test coverage for parameter binding.The integration tests cover the scenarios requested in the previous review:
- Positional placeholders with multiple types (null, bool, int, float, text)
- Numbered placeholders including duplicate indices and gap detection
- Blob via base64 object and bigint via string
packages/sqlite-web-core/src/worker.rs (2)
9-9: Import looks right.Brings in the canonical WorkerMessage type for typed deserialization.
41-47: Review comment is incorrect. Current match statement is already exhaustive and properly future-proofed.The
WorkerMessageenum contains only one variant (ExecuteQuery), making the current match exhaustive. With a single-variant enum, Rust's compiler requires all variants to be matched—the current code satisfies this requirement without a catch-all. TheErrarm is already present and properly handles deserialization errors.Adding an
Ok(_)catch-all would actually violate Rust's exhaustiveness checking principle. When a newWorkerMessagevariant is added later, the compiler will enforce that this match statement be updated. With a catch-all, new variants would silently be ignored—creating maintenance issues rather than preventing them. The current implementation correctly leverages Rust's type system to catch missing handlers at compile time.Likely an incorrect or invalid review comment.
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 (1)
packages/sqlite-web/src/lib.rs (1)
120-171: Out-of-order resolution risk: use requestId + map; also handle postMessage/worker-error.Current flow pairs requests/responses via LIFO pop without IDs; concurrent queries can resolve the wrong Promise. Also, worker-error messages are ignored and postMessage errors aren’t rejected.
Prefer correlating by a monotonically increasing requestId and storing callbacks in a HashMap. Reject on worker-error or send failure.
Option A (recommended): requestId correlation and robust error handling.
--- a/packages/sqlite-web/src/lib.rs +++ b/packages/sqlite-web/src/lib.rs @@ -use std::cell::RefCell; +use std::cell::{Cell, RefCell}; +use std::collections::HashMap; @@ pub struct SQLiteWasmDatabase { worker: Worker, - pending_queries: Rc<RefCell<Vec<(js_sys::Function, js_sys::Function)>>>, + pending_queries: Rc<RefCell<HashMap<u32, (js_sys::Function, js_sys::Function)>>>, + next_request_id: Rc<Cell<u32>>, } @@ - let pending_queries: Rc<RefCell<Vec<(js_sys::Function, js_sys::Function)>>> = - Rc::new(RefCell::new(Vec::new())); + let pending_queries: Rc<RefCell<HashMap<u32, (js_sys::Function, js_sys::Function)>>> = + Rc::new(RefCell::new(HashMap::new())); let pending_queries_clone = Rc::clone(&pending_queries); + let next_request_id = Rc::new(Cell::new(1u32)); + let next_request_id_clone = Rc::clone(&next_request_id); @@ - // Handle worker ready message + // Handle worker ready / error if let Ok(obj) = js_sys::Reflect::get(&data, &JsValue::from_str("type")) { if let Some(msg_type) = obj.as_string() { if msg_type == "worker-ready" { return; } else if msg_type == "worker-error" { - if let Ok(_error) = js_sys::Reflect::get(&data, &JsValue::from_str("error")) - { - } + let _ = if let Ok(id_val) = js_sys::Reflect::get(&data, &JsValue::from_str("requestId")) { + if let Some(id) = id_val.as_f64() { + pending_queries_clone + .borrow_mut() + .remove(&(id as u32)) + .and_then(|(_, reject)| { + let err = js_sys::Reflect::get(&data, &JsValue::from_str("error")).ok() + .and_then(|v| v.as_string()) + .unwrap_or_else(|| "Worker error".to_string()); + reject.call1(&JsValue::NULL, &JsValue::from_str(&err)).ok() + }); + } + }; return; } } } @@ - if msg_type == "query-result" { - if let Some((resolve, reject)) = pending_queries_clone.borrow_mut().pop() { + if msg_type == "query-result" { + let id_val = js_sys::Reflect::get(&data, &JsValue::from_str("requestId")).ok(); + if let Some(id_js) = id_val { + if let Some((resolve, reject)) = id_js.as_f64() + .and_then(|id| pending_queries_clone.borrow_mut().remove(&(id as u32))) { // Check for error first @@ - } + } + } } } } @@ - Ok(SQLiteWasmDatabase { - worker, - pending_queries, - }) + Ok(SQLiteWasmDatabase { worker, pending_queries, next_request_id }) } @@ pub async fn query( &self, sql: &str, params: Option<js_sys::Array>, ) -> Result<String, SQLiteWasmDatabaseError> { @@ - // Create the Promise that will resolve/reject when the worker responds - let promise = js_sys::Promise::new(&mut |resolve, reject| { - // Only store callbacks after successful message construction - pending_queries.borrow_mut().push((resolve, reject)); - - let _ = worker.post_message(&message); - }); + // Create the Promise that will resolve/reject when the worker responds + let promise = js_sys::Promise::new(&mut |resolve, reject| { + // Assign requestId + let id = self.next_request_id.get(); + self.next_request_id.set(id.wrapping_add(1)); + let _ = js_sys::Reflect::set(&message, &JsValue::from_str("requestId"), &JsValue::from_f64(id as f64)); + // Track resolver/rejector + pending_queries.borrow_mut().insert(id, (resolve.clone(), reject.clone())); + // Send and handle immediate failure + match worker.post_message(&message) { + Ok(()) => {} + Err(e) => { + pending_queries.borrow_mut().remove(&id); + let _ = reject.call1(&JsValue::NULL, &e); + } + } + });Option B (minimal): switch to FIFO (VecDeque, pop_front) to maintain order if worker guarantees ordered processing.
Also applies to: 326-333
♻️ Duplicate comments (1)
packages/sqlite-web-core/src/database.rs (1)
242-260: Handle u64 JSON numbers with range checks.serde_json::Number can be u64. Add an as_u64 branch; bind as int64 if <= i64::MAX, otherwise error clearly.
- serde_json::Value::Number(num) => { - if let Some(v) = num.as_i64() { + serde_json::Value::Number(num) => { + if let Some(v) = num.as_i64() { let rc = unsafe { sqlite3_bind_int64(stmt, i, v) }; @@ - } else if let Some(v) = num.as_f64() { + } else if let Some(u) = num.as_u64() { + if u <= i64::MAX as u64 { + let rc = unsafe { sqlite3_bind_int64(stmt, i, u as i64) }; + if rc != SQLITE_OK { + return Err(format!("Failed to bind int64 at {i}")); + } + } else { + return Err(format!( + "Unsigned integer at index {} exceeds i64 range.", + target_index + 1 + )); + } + } else if let Some(v) = num.as_f64() { // serde_json never represents NaN/Infinity let rc = unsafe { sqlite3_bind_double(stmt, i, v) };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
svelte-test/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
packages/sqlite-web-core/src/database.rs(5 hunks)packages/sqlite-web/src/lib.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web/worker_template.rs : Ensure the worker JavaScript generated by worker_template.rs embeds the WASM (base64) and does not fetch external WASM files
Applied to files:
packages/sqlite-web/src/lib.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web/lib.rs : Expose an async, promise-based query interface from the SQLiteWasmDatabase public API
Applied to files:
packages/sqlite-web/src/lib.rs
📚 Learning: 2025-10-20T12:24:07.846Z
Learnt from: findolor
PR: rainlanguage/sqlite-web#19
File: packages/sqlite-web-core/src/database.rs:122-157
Timestamp: 2025-10-20T12:24:07.846Z
Learning: In packages/sqlite-web-core/src/database.rs, named parameters (:name, name, $name) are intentionally not supported for SQL parameter binding. Only plain '?' and numbered '?N' placeholders are supported.
Applied to files:
packages/sqlite-web-core/src/database.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web-core/src/database_functions.rs : Integrate and expose rain.math.float-backed custom database functions for SQLite queries
Applied to files:
packages/sqlite-web-core/src/database.rs
🧬 Code graph analysis (2)
packages/sqlite-web/src/lib.rs (1)
packages/sqlite-web-core/src/coordination.rs (1)
new(25-59)
packages/sqlite-web-core/src/database.rs (1)
packages/sqlite-web/src/lib.rs (4)
new(88-180)v(206-206)v(225-225)v(242-242)
⏰ 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: test
- GitHub Check: test
🔇 Additional comments (1)
packages/sqlite-web-core/src/database.rs (1)
407-431: Good: RAII ensures stmt finalization on all paths.StmtGuard + BoundBuffers lifetime correctly prevent leaks on bind/exec errors. No action needed.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
svelte-test/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
packages/sqlite-web/src/lib.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web/worker_template.rs : Ensure the worker JavaScript generated by worker_template.rs embeds the WASM (base64) and does not fetch external WASM files
Applied to files:
packages/sqlite-web/src/lib.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web/lib.rs : Expose an async, promise-based query interface from the SQLiteWasmDatabase public API
Applied to files:
packages/sqlite-web/src/lib.rs
⏰ 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: test
- GitHub Check: test
🔇 Additional comments (5)
packages/sqlite-web/src/lib.rs (5)
1-1: LGTM: base64::Engine import added.Required for the parameter normalization that encodes binary data (Uint8Array/ArrayBuffer) as base64 strings for transmission to the worker.
182-289: Strong implementation with comprehensive type handling.The normalization correctly handles:
- Sparse array holes via
arr.get(i)returning undefined- All major JS types (primitives, BigInt, binary data)
- Proper error propagation throughout (past review concern addressed)
- NaN/Infinity rejection for numeric values
291-336: Well-structured query implementation with proper parameter handling.Key improvements:
- Optional parameters maintain backward compatibility
- Params field only included in message when non-empty (past review addressed)
- Consistent error propagation via
map_errthroughout message construction- Clear documentation of parameter handling behavior
457-505: Clever test helpers for message inspection.The spy pattern allows validation of message construction without requiring a fully functional worker. Proper cleanup via
uninstall_post_message_spy()and defensive try-catch for missing Worker API make this robust for the test environment.
507-671: Excellent test coverage addressing all past review suggestions.The tests comprehensively verify:
- All supported parameter types (primitives, BigInt, binary data)
- Sparse array holes normalize to SQL NULL (index 4 in first test)
- Message params field presence/absence behavior
- NaN/Infinity rejection for numeric values
The sparse array test construction (lines 522-537) correctly creates a hole at index 4 by pushing indices 0-3, then using
Reflect::setfor index 5, leaving 4 undefined.
| true | ||
| } | ||
|
|
||
| fn bind_params_for_stmt( |
There was a problem hiding this comment.
this function is seven screens long. can we, please, refactor it? Also, I reckon a lot of the mutability and looping can be replaced with functional-style iterator method chaining, which could be easier to read/comprehend, so consider using that as well
There was a problem hiding this comment.
is_trivia_tail_only is not quite seven screens long but can probably be refactored to be easier to comprehend as well
| match db_opt { | ||
| Some(mut database) => { | ||
| let result = database.exec(&sql).await; | ||
| let result = match params { |
There was a problem hiding this comment.
this is a lot of levels of nesting. consider refactoring to reduce
| let _ = worker_scope.post_message(&response); | ||
| }); | ||
| // Deserialize into canonical WorkerMessage and handle variants | ||
| match serde_wasm_bindgen::from_value::<WorkerMessage>(data) { |
There was a problem hiding this comment.
i see multiple .unwrap's in here. this isn't test code, is it?
| } | ||
|
|
||
| /// Execute a SQL query | ||
| fn normalize_params_js(params: &JsValue) -> Result<js_sys::Array, SQLiteWasmDatabaseError> { |
There was a problem hiding this comment.
this one is nearly three screens long. please, split it up to make it easier to process. also, consider fp/iterator/immutability refactorings, that might also help readability
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sqlite-web-core/src/database.rs (1)
1-1842: Consider file refactoring to address length concerns.The parameter binding implementation is functionally correct and well-tested. However, past reviews noted concerns about file length ("seven screens long") and suggested refactoring with functional-style iterator chaining where appropriate. The file has grown to 1842 lines. Consider splitting into separate modules:
database/core.rs- core database operationsdatabase/params.rs- parameter parsing and bindingdatabase/placeholders.rs- placeholder detection and validationdatabase/execution.rs- statement execution logicThis would improve maintainability without changing functionality.
Based on learnings.
♻️ Duplicate comments (2)
packages/sqlite-web-core/src/messages.rs (1)
62-101: Add a test with params = Some([...]) to assert roundtrip.Covers the positive path for optional params serialization.
+ #[wasm_bindgen_test] + fn test_query_request_with_params_roundtrip() { + let msg = ChannelMessage::QueryRequest { + query_id: "q1".into(), + sql: "SELECT ?1, ?2".into(), + params: Some(vec![serde_json::json!(1), serde_json::json!("a")]), + }; + assert_serialization_roundtrip(msg, "query-request", |json| { + assert!(json.contains("\"params\":[1,\"a\"]")); + }); + } + + #[wasm_bindgen_test] + fn test_worker_execute_query_with_params_roundtrip() { + let msg = WorkerMessage::ExecuteQuery { + sql: "SELECT ?".into(), + params: Some(vec![serde_json::json!({"k": "v"})]), + }; + let s = serde_json::to_string(&msg).unwrap(); + let back: WorkerMessage = serde_json::from_str(&s).unwrap(); + assert_eq!(msg, back); + }Also applies to: 125-142, 168-191
packages/sqlite-web-core/src/worker.rs (1)
98-115: Refactor error classification: emit worker-error, not query-result, for malformed inbound messages.The concern is valid. Deserialization failures occur before any user query is associated, so they should not emit
query-resultmessages. The current code causes the main thread to pop frompending_queries(line 66-67 in lib.rs) for an error unrelated to any pending query, creating a queue mismatch where the wrong resolver receives the error and actual pending queries hang.The proposed refactor is correct:
send_worker_error_messageis available (defined at worker.rs:19) and already used throughout- Main thread correctly ignores
worker-errorwithout queue interaction (lib.rs:51)- The simplified
returnavoids the fallback chainApply the suggested diff:
- Err(err) => { - let error_text = format!("Invalid worker message: {err:?}"); - let response = make_query_result_message(Err(error_text)).unwrap_or_else(|e| { - let _ = send_worker_error(e); - let o = js_sys::Object::new(); - let _ = set_js_property(&o, "type", &JsValue::from_str("query-result")); - let _ = set_js_property(&o, "result", &JsValue::NULL); - let _ = - set_js_property(&o, "error", &JsValue::from_str("Failed to build response")); - o - }); - if let Err(err) = post_message(&response) { - if let Err(send_err) = send_worker_error(err) { - throw_val(send_err); - } - } - } + Err(err) => { + let _ = send_worker_error_message(&format!("Invalid worker message: {err:?}")); + return; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
svelte-test/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
packages/sqlite-web-core/src/coordination.rs(12 hunks)packages/sqlite-web-core/src/database.rs(11 hunks)packages/sqlite-web-core/src/database_functions/bigint_sum.rs(3 hunks)packages/sqlite-web-core/src/database_functions/float_is_zero.rs(1 hunks)packages/sqlite-web-core/src/database_functions/float_negate.rs(1 hunks)packages/sqlite-web-core/src/database_functions/float_sum.rs(3 hunks)packages/sqlite-web-core/src/database_functions/mod.rs(6 hunks)packages/sqlite-web-core/src/lib.rs(0 hunks)packages/sqlite-web-core/src/messages.rs(8 hunks)packages/sqlite-web-core/src/util.rs(2 hunks)packages/sqlite-web-core/src/worker.rs(4 hunks)packages/sqlite-web/src/lib.rs(6 hunks)
💤 Files with no reviewable changes (1)
- packages/sqlite-web-core/src/lib.rs
🧰 Additional context used
📓 Path-based instructions (2)
packages/sqlite-web-core/src/worker.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Keep worker.rs as the main worker entry point invoked by worker_main()
Files:
packages/sqlite-web-core/src/worker.rs
packages/sqlite-web-core/src/messages.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use structured message types for worker communication as defined in messages.rs
Files:
packages/sqlite-web-core/src/messages.rs
🧠 Learnings (8)
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web-core/src/messages.rs : Use structured message types for worker communication as defined in messages.rs
Applied to files:
packages/sqlite-web-core/src/worker.rspackages/sqlite-web-core/src/messages.rspackages/sqlite-web/src/lib.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web-core/src/worker.rs : Keep worker.rs as the main worker entry point invoked by worker_main()
Applied to files:
packages/sqlite-web-core/src/worker.rspackages/sqlite-web-core/src/coordination.rspackages/sqlite-web/src/lib.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web/worker_template.rs : Ensure the worker JavaScript generated by worker_template.rs embeds the WASM (base64) and does not fetch external WASM files
Applied to files:
packages/sqlite-web-core/src/worker.rspackages/sqlite-web-core/src/coordination.rspackages/sqlite-web/src/lib.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web-core/src/database_functions.rs : Integrate and expose rain.math.float-backed custom database functions for SQLite queries
Applied to files:
packages/sqlite-web-core/src/database_functions/mod.rspackages/sqlite-web-core/src/database_functions/float_negate.rspackages/sqlite-web-core/src/database.rspackages/sqlite-web-core/src/database_functions/float_is_zero.rspackages/sqlite-web-core/src/database_functions/float_sum.rs
📚 Learning: 2025-09-19T07:06:37.309Z
Learnt from: findolor
PR: rainlanguage/sqlite-web#14
File: svelte-test/tests/database-functions/float-is-zero.test.ts:60-90
Timestamp: 2025-09-19T07:06:37.309Z
Learning: For floating-point database functions in the rainlanguage/sqlite-web codebase, negative zero handling is not required and doesn't need to be tested.
Applied to files:
packages/sqlite-web-core/src/database_functions/float_negate.rspackages/sqlite-web-core/src/database_functions/float_is_zero.rs
📚 Learning: 2025-10-20T12:24:07.874Z
Learnt from: findolor
PR: rainlanguage/sqlite-web#19
File: packages/sqlite-web-core/src/database.rs:122-157
Timestamp: 2025-10-20T12:24:07.874Z
Learning: In packages/sqlite-web-core/src/database.rs, named parameters (:name, name, $name) are intentionally not supported for SQL parameter binding. Only plain '?' and numbered '?N' placeholders are supported.
Applied to files:
packages/sqlite-web-core/src/database.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web/lib.rs : Expose an async, promise-based query interface from the SQLiteWasmDatabase public API
Applied to files:
packages/sqlite-web-core/src/database.rspackages/sqlite-web-core/src/coordination.rspackages/sqlite-web/src/lib.rs
📚 Learning: 2025-08-27T05:55:40.481Z
Learnt from: findolor
PR: rainlanguage/sqlite-web#5
File: packages/sqlite-web-core/src/database_functions/bigint_sum.rs:76-93
Timestamp: 2025-08-27T05:55:40.481Z
Learning: In the sqlite-web-core codebase, for SQLite aggregate function context initialization, the maintainer prefers using byte-scanning to detect zero-initialized memory rather than using explicit initialization flags or Option wrappers.
Applied to files:
packages/sqlite-web-core/src/database_functions/bigint_sum.rspackages/sqlite-web-core/src/database_functions/float_sum.rs
🧬 Code graph analysis (3)
packages/sqlite-web-core/src/worker.rs (2)
packages/sqlite-web-core/src/util.rs (2)
js_value_to_string(39-50)set_js_property(29-37)packages/sqlite-web-core/src/coordination.rs (2)
new(44-78)setup_channel_listener(80-97)
packages/sqlite-web-core/src/database_functions/float_sum.rs (1)
packages/sqlite-web-core/src/database_functions/bigint_sum.rs (5)
new(15-17)std(81-81)std(99-99)std(128-131)std(153-156)
packages/sqlite-web-core/src/coordination.rs (2)
packages/sqlite-web-core/src/util.rs (4)
js_value_to_string(39-50)sanitize_identifier(5-19)set_js_property(29-37)value(43-43)packages/sqlite-web/src/lib.rs (5)
new(226-244)from(202-204)from(208-210)from(214-219)query(276-317)
⏰ 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: test
- GitHub Check: test
🔇 Additional comments (33)
packages/sqlite-web-core/src/database_functions/float_negate.rs (1)
97-97: LGTM! Test configuration correctly aligned with wasm-bindgen-test framework.The change to
#[cfg(all(test, target_family = "wasm"))]appropriately restricts test compilation to WebAssembly targets, which matches the test framework being used (wasm_bindgen_test). This prevents build errors on non-WASM targets where wasm-bindgen-test dependencies wouldn't be available.packages/sqlite-web/src/lib.rs (4)
120-151: LGTM: Comprehensive parameter type handling.The function correctly handles all JavaScript parameter types including null/undefined, BigInt, binary data (Uint8Array/ArrayBuffer), primitives (number/boolean/string), with proper validation for non-finite numbers and clear error messages.
90-118: LGTM: Clean encoding helpers with proper error handling.Both
encode_bigint_to_objandencode_binary_to_objcorrectly serialize JavaScript values with appropriate error propagation usingmap_err.
488-652: LGTM: Comprehensive test coverage for parameter binding.The test suite thoroughly covers:
- All parameter types (primitives, BigInt, binary data)
- Edge cases (sparse arrays with holes, NaN/Infinity rejection)
- Message format validation (params presence/absence)
- Proper assertions on normalized output
The post-message spy mechanism is a clever approach to validate worker message construction without requiring a full worker implementation.
15-44: LGTM: Clean worker creation and message handler setup.The
create_worker_from_codefunction properly creates a Worker from embedded code using the Blob API and correctly revokes the object URL to prevent memory leaks. Theinstall_onmessage_handlerappropriately delegates to control and result handlers.packages/sqlite-web-core/src/util.rs (1)
29-37: LGTM: robust wrapper over Reflect::set.Clear Ok(false) path and propagated JsValue error are good for debugging interop failures.
packages/sqlite-web-core/src/database_functions/mod.rs (1)
129-156: wasm-only tests are fine.Gating tests to wasm matches where these modules run.
packages/sqlite-web-core/src/database_functions/float_is_zero.rs (1)
58-91: LGTM: wasm-targeted tests.Scope is narrowed appropriately for wasm builds.
packages/sqlite-web-core/src/database_functions/bigint_sum.rs (2)
7-7: Nice: zero-result avoids allocation.Static "0" bytes with correct length and TRANSIENT use are sound.
Also applies to: 123-133
94-111: Context init check matches project style.Zeroed-memory scan aligns with prior preference. (Based on learnings)
packages/sqlite-web-core/src/database_functions/float_sum.rs (1)
6-6: Good defensive handling for zero-rows case.Creating zero hex and surfacing a clear error on CString failure is robust.
Also applies to: 106-129
packages/sqlite-web-core/src/messages.rs (1)
18-21: Params shape is backward-compatible and correct.serde(default) + skip_serializing_if ensure omission on None and resilience on missing fields. (As per coding guidelines)
Also applies to: 36-41
packages/sqlite-web-core/src/worker.rs (2)
48-62: Structured result builder is clean.make_query_result_message centralizes shape; fallback path also reports worker-error.
124-133: Startup error reporting is solid.Propagating leadership errors via worker-error and throwing on failure is appropriate.
packages/sqlite-web-core/src/database.rs (12)
18-46: LGTM: RAII guard and buffer lifetime management.The
BoundBuffersandStmtGuardimplementations correctly handle lifetime management for bound text/blob data and ensure proper statement finalization. TheStmtGuard::take()method properly prevents double-finalization by nulling the pointer.
114-164: LGTM: Column name and value extraction.The column handling methods correctly extract names and values with appropriate fallbacks for edge cases (missing names, invalid floats).
166-256: LGTM: Placeholder detection and validation.The placeholder mode detection and validation correctly enforces the constraint that only plain '?' and numbered '?N' placeholders are supported (mixing is disallowed, named parameters are rejected). Continuity validation for numbered placeholders ensures proper parameter mapping.
Based on learnings.
258-295: LGTM: Parameter mapping logic.The
build_param_mapmethod correctly maps SQLite parameter indices to user-provided parameter indices for both plain and numbered placeholder modes. Theunreachable!()at lines 271-272 is justified since numbered placeholders never return null names.
297-359: LGTM: JSON parameter parsing.The parameter parsing methods correctly handle various JSON types including extended types (blob via base64, bigint as string). Error messages include 1-based parameter indices for clarity.
361-454: LGTM: Parameter binding with proper lifetime management.The binding methods correctly bind various parameter types to SQLite statements. Text and blob bindings store buffers in
BoundBuffersand pass pointers to SQLite withNonedestructor, relying onBoundBufferslifetime to keep the data alive during statement execution.
455-502: LGTM: Tail trivia validation.The
is_trivia_tail_onlyfunction correctly identifies whether the tail pointer contains only whitespace and comments (line/block). The implementation appropriately assumes the tail begins after a complete SQL statement, so string/quote handling is not required.
504-579: LGTM: Parameterized statement execution.The
bind_params_for_stmtandexec_single_statement_with_paramsmethods correctly orchestrate parameter binding and execution. The use ofStmtGuardensures proper statement finalization even on error paths. The single-statement constraint is appropriately enforced for parameterized queries.
640-687: LGTM: Updated statement execution with lazy column names.The modifications to
exec_prepared_statementcorrectly useStmtGuardfor automatic cleanup and lazily collect column names only when the first row is encountered (optimization for non-SELECT statements).
690-797: LGTM: Transaction state management improvements.The updates to
execandexec_single_statementcorrectly integrate transaction state tracking and automatic rollback on error, preventing orphaned transactions.
799-817: LGTM: New parameterized query API.The
exec_with_paramspublic method provides a clean interface for parameterized queries, properly delegating to the internal implementation and handling transaction state refresh.
1062-1218: LGTM: Comprehensive test coverage for parameter binding.The integration tests thoroughly cover parameterized queries including:
- Various data types (null, bool, int, float, text)
- Plain and numbered placeholder modes
- Duplicate numbered indices
- Gap detection in numbered placeholders
- Extended types (blob via base64, bigint as string)
packages/sqlite-web-core/src/coordination.rs (7)
25-41: LGTM: Helper functions for cleaner code.The
reflect_getandsend_channel_messagehelpers provide clean abstractions that centralize error handling and improve readability throughout the file.
80-162: LGTM: Improved error propagation in setup methods.The updated
setup_channel_listenerandattempt_leadershipmethods now returnResult<(), JsValue>for proper error propagation. The use of helper functions (reflect_get,set_js_property,send_channel_message) improves code clarity and error handling.
164-200: LGTM: Parameter support in execute_query.The updated
execute_querymethod correctly threads the optionalparamsthrough both leader (direct execution) and follower (channel message) code paths. The extracted timeout logic improves readability.
203-240: LGTM: Centralized message handler.The
handle_channel_messagefunction provides clean centralized routing for channel messages with proper error handling and fallback responses.
242-294: LGTM: Clean helper functions for query execution.The
build_query_response,handle_query_response, andexec_on_dbfunctions provide clear, focused abstractions that improve code organization. Theexec_on_dbfunction correctly routes toexec_with_paramsorexecbased on whether parameters are provided.
296-340: LGTM: Query request and timeout handling.The
post_query_requestandschedule_timeout_promisefunctions correctly handle message posting and timeout management. The timeout promise properly cleans up pending queries.Note: Past reviews mentioned "a lot of levels of nesting" in this file. While the current nesting in these functions is moderate, consider flattening nested closures if you refactor for readability.
466-546: LGTM: Test updates for new signatures.The test updates correctly handle the new method signatures, passing
Nonefor parameters where appropriate and checkingResultreturn types for setup methods.
| let bigint_sum_name = CString::new("BIGINT_SUM") | ||
| .map_err(|_| "Function name BIGINT_SUM contains interior NUL bytes".to_string())?; | ||
| let ret = unsafe { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid allocation for static function names.
Use CStr::from_bytes_with_nul on b"...\0" to skip CString allocation and impossible interior‑NUL error on string literals. Keeps intent and reduces overhead.
- let bigint_sum_name = CString::new("BIGINT_SUM")
- .map_err(|_| "Function name BIGINT_SUM contains interior NUL bytes".to_string())?;
+ let bigint_sum_name = std::ffi::CStr::from_bytes_with_nul(b"BIGINT_SUM\0").unwrap();
...
- bigint_sum_name.as_ptr(),
+ bigint_sum_name.as_ptr(),Apply similarly to FLOAT_SUM, FLOAT_ZERO_HEX, FLOAT_NEGATE, FLOAT_IS_ZERO. (Based on learnings)
Also applies to: 43-45, 64-66, 85-87, 106-108
🤖 Prompt for AI Agents
In packages/sqlite-web-core/src/database_functions/mod.rs around lines 22-24,
the code creates CString allocations for static function names (e.g.
"BIGINT_SUM"); change these to use CStr::from_bytes_with_nul(b"BIGINT_SUM\0") to
avoid allocation and eliminate the possibility of interior NUL errors for string
literals, converting to a pointer as needed for the unsafe call; apply the same
change to the other static names listed at lines 43-45, 64-66, 85-87, and
106-108 (FLOAT_SUM, FLOAT_ZERO_HEX, FLOAT_NEGATE, FLOAT_IS_ZERO).
There was a problem hiding this comment.
@findolor, understood! I'll skip this optimization. The current approach with CString::new and proper error handling works well.
🧠 Learnings used
Learnt from: CR
PR: rainlanguage/sqlite-web#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web-core/src/database_functions.rs : Integrate and expose rain.math.float-backed custom database functions for SQLite queries
| fn sqlite_error_from_code(&self, code: i32) -> String { | ||
| unsafe { | ||
| let p = sqlite3_errmsg(self.db); | ||
| if !p.is_null() { | ||
| CStr::from_ptr(p).to_string_lossy().into_owned() | ||
| } else { | ||
| format!("SQLite error code: {code}") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn refresh_transaction_state(&mut self) { | ||
| self.in_transaction = unsafe { sqlite3_get_autocommit(self.db) } == 0; | ||
| } | ||
|
|
||
| async fn rollback_if_in_transaction(&self) { | ||
| if unsafe { sqlite3_get_autocommit(self.db) } == 0 { | ||
| let _ = self.exec_single_statement("ROLLBACK").await; | ||
| } | ||
| } | ||
|
|
||
| fn prepare_one( | ||
| &self, | ||
| ptr: *const i8, | ||
| ) -> Result<(Option<*mut sqlite3_stmt>, *const i8), String> { | ||
| let mut stmt: *mut sqlite3_stmt = std::ptr::null_mut(); | ||
| let mut tail: *const i8 = std::ptr::null(); | ||
| let ret = unsafe { sqlite3_prepare_v2(self.db, ptr, -1, &mut stmt, &mut tail) }; | ||
| if ret != SQLITE_OK { | ||
| return Err(self.sqlite_error_from_code(ret)); | ||
| } | ||
| Ok((if stmt.is_null() { None } else { Some(stmt) }, tail)) | ||
| } | ||
| fn sqlite_errmsg(&self) -> String { | ||
| unsafe { | ||
| let p = sqlite3_errmsg(self.db); | ||
| if !p.is_null() { | ||
| CStr::from_ptr(p).to_string_lossy().into_owned() | ||
| } else { | ||
| "Unknown SQLite error".to_string() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consolidate duplicate error message methods.
The methods sqlite_error_from_code (lines 70-79) and sqlite_errmsg (lines 103-112) are nearly identical - both retrieve the error message from sqlite3_errmsg. Consider consolidating them into a single method.
Apply this diff to consolidate:
- fn sqlite_error_from_code(&self, code: i32) -> String {
+ fn sqlite_errmsg(&self) -> String {
unsafe {
let p = sqlite3_errmsg(self.db);
if !p.is_null() {
CStr::from_ptr(p).to_string_lossy().into_owned()
} else {
- format!("SQLite error code: {code}")
+ "Unknown SQLite error".to_string()
}
}
}
-
- fn sqlite_errmsg(&self) -> String {
- unsafe {
- let p = sqlite3_errmsg(self.db);
- if !p.is_null() {
- CStr::from_ptr(p).to_string_lossy().into_owned()
- } else {
- "Unknown SQLite error".to_string()
- }
- }
- }Then update call sites to use sqlite_errmsg() instead of sqlite_error_from_code().
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/sqlite-web-core/src/database.rs around lines 70 to 112, two nearly
identical methods sqlite_error_from_code and sqlite_errmsg both call
sqlite3_errmsg; consolidate them by removing sqlite_error_from_code and ensuring
sqlite_errmsg returns the same behavior (prefer the sqlite3_errmsg string when
non-null, otherwise return a fallback that includes the numeric code when
available or a generic message), then update all call sites to call
sqlite_errmsg() instead of sqlite_error_from_code(code) and adjust callers to
pass the error code into the fallback message only if needed.
| pub fn js_value_to_string(value: &JsValue) -> String { | ||
| if let Some(s) = value.as_string() { | ||
| return s; | ||
| } | ||
| if let Some(error) = value.dyn_ref::<js_sys::Error>() { | ||
| return error | ||
| .to_string() | ||
| .as_string() | ||
| .unwrap_or_else(|| format!("{value:?}")); | ||
| } | ||
| format!("{value:?}") | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Improve error stringification fallback for non-Error JsValues.
Before falling back to Debug, try JSON.stringify to capture objects cleanly.
pub fn js_value_to_string(value: &JsValue) -> String {
if let Some(s) = value.as_string() {
return s;
}
if let Some(error) = value.dyn_ref::<js_sys::Error>() {
return error
.to_string()
.as_string()
.unwrap_or_else(|| format!("{value:?}"));
}
+ if let Ok(s) = js_sys::JSON::stringify(value) {
+ if let Some(s) = s.as_string() {
+ return s;
+ }
+ }
format!("{value:?}")
}📝 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.
| pub fn js_value_to_string(value: &JsValue) -> String { | |
| if let Some(s) = value.as_string() { | |
| return s; | |
| } | |
| if let Some(error) = value.dyn_ref::<js_sys::Error>() { | |
| return error | |
| .to_string() | |
| .as_string() | |
| .unwrap_or_else(|| format!("{value:?}")); | |
| } | |
| format!("{value:?}") | |
| } | |
| pub fn js_value_to_string(value: &JsValue) -> String { | |
| if let Some(s) = value.as_string() { | |
| return s; | |
| } | |
| if let Some(error) = value.dyn_ref::<js_sys::Error>() { | |
| return error | |
| .to_string() | |
| .as_string() | |
| .unwrap_or_else(|| format!("{value:?}")); | |
| } | |
| if let Ok(s) = js_sys::JSON::stringify(value) { | |
| if let Some(s) = s.as_string() { | |
| return s; | |
| } | |
| } | |
| format!("{value:?}") | |
| } |
🤖 Prompt for AI Agents
In packages/sqlite-web-core/src/util.rs around lines 39 to 50, the fallback for
non-Error JsValues should try JSON.stringify before falling back to Debug;
modify the function so after checking for string and js_sys::Error you call
js_sys::JSON::stringify(value) and, if it succeeds, convert the resulting
JsString to Rust String (returning that when Some), otherwise fall back to
formatting with Debug; ensure you handle stringify's Result (map the Ok path to
as_string().unwrap_or_else(|| format!("{value:?}")) and on Err return
format!("{value:?}")).
| fn handle_worker_control_message(data: &JsValue) -> bool { | ||
| if let Ok(obj) = js_sys::Reflect::get(data, &JsValue::from_str("type")) { | ||
| if let Some(msg_type) = obj.as_string() { | ||
| if msg_type == "worker-ready" { | ||
| return true; | ||
| } else if msg_type == "worker-error" { | ||
| let _ = js_sys::Reflect::get(data, &JsValue::from_str("error")); | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| false | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider logging worker errors.
Line 52 fetches the error value from the worker message but doesn't use it. While the function correctly returns true to indicate a control message was handled, logging the error would help with debugging.
} else if msg_type == "worker-error" {
- let _ = js_sys::Reflect::get(data, &JsValue::from_str("error"));
+ if let Ok(error) = js_sys::Reflect::get(data, &JsValue::from_str("error")) {
+ web_sys::console::error_1(&error);
+ }
return true;🤖 Prompt for AI Agents
In packages/sqlite-web/src/lib.rs around lines 46 to 58, the code reads the
"error" field from the worker message but drops it; modify the branch that
handles "worker-error" to log the error value before returning true. Retrieve
the error JsValue, convert it to a readable string (e.g., try error.as_string()
and fall back to js_sys::JSON::stringify(&error).ok().and_then(|s|
s.as_string()).unwrap_or_else(|| format!("{:?}", error))), then call
web_sys::console::error_1(&JsValue::from_str(&error_string)) so the error is
visible in the browser console.
| fn handle_query_result_message( | ||
| data: &JsValue, | ||
| pending_queries: &Rc<RefCell<Vec<(js_sys::Function, js_sys::Function)>>>, | ||
| ) { | ||
| if let Ok(obj) = js_sys::Reflect::get(data, &JsValue::from_str("type")) { | ||
| if let Some(msg_type) = obj.as_string() { | ||
| if msg_type == "query-result" { | ||
| if let Some((resolve, reject)) = pending_queries.borrow_mut().pop() { | ||
| if let Ok(error) = js_sys::Reflect::get(data, &JsValue::from_str("error")) { | ||
| if !error.is_null() && !error.is_undefined() { | ||
| let error_str = | ||
| error.as_string().unwrap_or_else(|| format!("{error:?}")); | ||
| let _ = reject.call1(&JsValue::NULL, &JsValue::from_str(&error_str)); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| if let Ok(result) = js_sys::Reflect::get(data, &JsValue::from_str("result")) { | ||
| if !result.is_null() && !result.is_undefined() { | ||
| let result_str = | ||
| result.as_string().unwrap_or_else(|| format!("{result:?}")); | ||
| let _ = resolve.call1(&JsValue::NULL, &JsValue::from_str(&result_str)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle failures when calling resolve/reject callbacks.
Lines 72 and 81 ignore the result of call1() on the resolve and reject functions. If these JavaScript function calls fail, the promise will never settle, causing the query to hang indefinitely without any indication of what went wrong.
Apply this diff to log failures:
- let _ = reject.call1(&JsValue::NULL, &JsValue::from_str(&error_str));
+ if let Err(e) = reject.call1(&JsValue::NULL, &JsValue::from_str(&error_str)) {
+ web_sys::console::error_1(&JsValue::from_str(&format!("Failed to reject promise: {e:?}")));
+ }
return;- let _ = resolve.call1(&JsValue::NULL, &JsValue::from_str(&result_str));
+ if let Err(e) = resolve.call1(&JsValue::NULL, &JsValue::from_str(&result_str)) {
+ web_sys::console::error_1(&JsValue::from_str(&format!("Failed to resolve promise: {e:?}")));
+ }🤖 Prompt for AI Agents
In packages/sqlite-web/src/lib.rs around lines 60 to 88, the code currently
ignores the Result returned by resolve.call1 and reject.call1 which can cause
promises to hang silently; update both call1 invocations to handle their Result
(e.g., match or if let Err(err) = ...) and log any failure using a JS console
logger such as web_sys::console::error_1(&err) or similar so failures are
visible; ensure both the reject and resolve branches handle and log errors from
call1 rather than discarding them.
| fn normalize_params_js(params: &JsValue) -> Result<js_sys::Array, SQLiteWasmDatabaseError> { | ||
| // If undefined or null, treat as no params | ||
| if params.is_undefined() || params.is_null() { | ||
| return Ok(js_sys::Array::new()); | ||
| } | ||
|
|
||
| // Ensure array input | ||
| let arr: js_sys::Array = if js_sys::Array::is_array(params) { | ||
| params.clone().unchecked_into() | ||
| } else { | ||
| return Err(SQLiteWasmDatabaseError::JsError(JsValue::from_str( | ||
| "params must be an array", | ||
| ))); | ||
| }; | ||
|
|
||
| let normalized = js_sys::Array::new(); | ||
| let len = arr.length(); | ||
| for i in 0..len { | ||
| let v = arr.get(i); | ||
| let nv = normalize_one_param(&v, i)?; | ||
| normalized.push(&nv); | ||
| } | ||
|
|
||
| Ok(normalized) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider refactoring for readability.
The normalize_params_js function handles array validation, iteration, and element normalization in a single imperative block. As noted by reviewer 0xgleb, this could be split or refactored using a more functional style for improved readability and maintainability.
Consider using an iterator-based approach:
fn normalize_params_js(params: &JsValue) -> Result<js_sys::Array, SQLiteWasmDatabaseError> {
if params.is_undefined() || params.is_null() {
return Ok(js_sys::Array::new());
}
let arr: js_sys::Array = if js_sys::Array::is_array(params) {
params.clone().unchecked_into()
} else {
return Err(SQLiteWasmDatabaseError::JsError(JsValue::from_str(
"params must be an array",
)));
};
let normalized = js_sys::Array::new();
for i in 0..arr.length() {
let v = arr.get(i);
let nv = normalize_one_param(&v, i)?;
normalized.push(&nv);
}
Ok(normalized)
}Alternatively, extract the array validation into a helper function.
🤖 Prompt for AI Agents
In packages/sqlite-web/src/lib.rs around lines 246-270, the normalize_params_js
function mixes param validation, array extraction, and element normalization in
one block; extract responsibilities to improve readability by (1) adding a small
helper like ensure_array(params: &JsValue) -> Result<js_sys::Array,
SQLiteWasmDatabaseError> that returns an empty Array for null/undefined or
errors if not an array, and (2) replacing the inline loop with a clearer
iteration over the returned Array (e.g., iterate 0..len and map each element
through normalize_one_param, pushing results into a new js_sys::Array or using a
concise iterator+for_each), wiring up error propagation with ? so behavior stays
identical.
| // Create the Promise that will resolve/reject when the worker responds | ||
| let promise = js_sys::Promise::new(&mut |resolve, reject| { | ||
| // Store the promise callbacks | ||
| // Only store callbacks after successful message construction | ||
| pending_queries.borrow_mut().push((resolve, reject)); | ||
|
|
||
| // Send query to worker - create JavaScript object directly | ||
| let message = js_sys::Object::new(); | ||
| js_sys::Reflect::set( | ||
| &message, | ||
| &JsValue::from_str("type"), | ||
| &JsValue::from_str("execute-query"), | ||
| ) | ||
| .unwrap(); | ||
| js_sys::Reflect::set( | ||
| &message, | ||
| &JsValue::from_str("sql"), | ||
| &JsValue::from_str(&sql), | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| let _ = worker.post_message(&message); | ||
| }); |
There was a problem hiding this comment.
Handle post_message failure to prevent hanging promises.
Line 312 ignores the result of worker.post_message(). If posting fails, the promise will never resolve because no worker response will arrive, yet the resolve/reject callbacks have already been added to pending_queries at line 310. This causes the query to hang indefinitely.
Apply this diff to handle the error:
let promise = js_sys::Promise::new(&mut |resolve, reject| {
- // Only store callbacks after successful message construction
pending_queries.borrow_mut().push((resolve, reject));
-
- let _ = worker.post_message(&message);
});
+
+ // Post message after promise creation; if it fails, reject the promise
+ if let Err(e) = worker.post_message(&message) {
+ if let Some((_, reject)) = pending_queries.borrow_mut().pop() {
+ let _ = reject.call1(&JsValue::NULL, &e);
+ }
+ return Err(SQLiteWasmDatabaseError::JsError(e));
+ }📝 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.
| // Create the Promise that will resolve/reject when the worker responds | |
| let promise = js_sys::Promise::new(&mut |resolve, reject| { | |
| // Store the promise callbacks | |
| // Only store callbacks after successful message construction | |
| pending_queries.borrow_mut().push((resolve, reject)); | |
| // Send query to worker - create JavaScript object directly | |
| let message = js_sys::Object::new(); | |
| js_sys::Reflect::set( | |
| &message, | |
| &JsValue::from_str("type"), | |
| &JsValue::from_str("execute-query"), | |
| ) | |
| .unwrap(); | |
| js_sys::Reflect::set( | |
| &message, | |
| &JsValue::from_str("sql"), | |
| &JsValue::from_str(&sql), | |
| ) | |
| .unwrap(); | |
| let _ = worker.post_message(&message); | |
| }); | |
| // Create the Promise that will resolve/reject when the worker responds | |
| let promise = js_sys::Promise::new(&mut |resolve, reject| { | |
| // Try to post the message first; only store callbacks if successful | |
| if let Err(e) = worker.post_message(&message) { | |
| let _ = reject.call1(&JsValue::NULL, &e); | |
| return; | |
| } | |
| // Only store callbacks after successful message posting | |
| pending_queries.borrow_mut().push((resolve, reject)); | |
| }); |
🤖 Prompt for AI Agents
In packages/sqlite-web/src/lib.rs around lines 307 to 313, the code currently
pushes resolve/reject into pending_queries before calling worker.post_message,
which can leave a dangling promise if post_message fails; change the flow so you
attempt post_message first and only push the callbacks into pending_queries if
post_message succeeds, and if post_message returns an error immediately call
reject (and do not push) so the Promise rejects instead of hanging; concretely,
capture the result of worker.post_message, check for error, invoke reject with
the error (and return) on failure, and only push (resolve, reject) into
pending_queries after a successful post_message.
3 participants
Motivation
See issue: #18
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Tests
Chores