Implement parameter binding for the query function #19
Conversation
WalkthroughAdds optional SQL query parameters end-to-end: JS-side param normalization (BigInt, binary→base64), marshaling of Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(240,248,255,0.6)
participant App as App / Test
participant JS as JS API (query)
participant Norm as normalize_params_js
participant Worker as Worker (embedded)
participant Coord as Coordination
participant DB as SQLiteDatabase
end
App->>JS: query(sql, params?)
JS->>Norm: normalize params (BigInt, Uint8Array/ArrayBuffer→base64)
Norm-->>JS: Option<Array>
alt Leader (in-process)
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 (via Worker)
JS->>Worker: post QueryRequest { query_id, 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 { requestId, result|null, error|null }
end
JS-->>App: resolve Promise / reject with error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
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)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (2)packages/sqlite-web-core/src/worker.rs📄 CodeRabbit inference engine (CLAUDE.md)
Files:
packages/sqlite-web-core/src/messages.rs📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (4)📚 Learning: 2025-09-17T06:55:20.178ZApplied to files:
📚 Learning: 2025-09-17T06:55:20.178ZApplied to files:
📚 Learning: 2025-09-17T06:55:20.178ZApplied to files:
📚 Learning: 2025-09-17T06:55:20.178ZApplied to files:
🧬 Code graph analysis (2)packages/sqlite-web-core/src/worker.rs (2)
packages/sqlite-web/src/lib.rs (1)
🔇 Additional comments (15)
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?
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 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.
There was a problem hiding this comment.
@findolor, understood! I'll skip this suggestion.
🧠 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/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-core/src/messages.rs : Use structured message types for worker communication as defined in messages.rs
| 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.
There was a problem hiding this comment.
@findolor, understood. I've noted your decision on this suggestion.
🧠 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/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-core/src/messages.rs : Use structured message types for worker communication as defined in messages.rs
There was a problem hiding this comment.
Actionable comments posted: 8
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)
1368-1381: Fix test assertion to match actual prepare error message.prepare_one returns the SQLite error string (e.g., “near ...: syntax error”). Adjust the test to assert on that instead of a hardcoded prefix.
let result = db.exec("INVALID SQL SYNTAX HERE").await; assert!(result.is_err()); let error = result.unwrap_err(); - assert!( - error.contains("Failed to prepare statement"), - "Invalid SQL should produce prepare statement error" - ); + assert!( + error.contains("syntax error") || error.to_lowercase().contains("sqlite error"), + "Invalid SQL should surface SQLite's prepare error" + );
♻️ Duplicate comments (2)
packages/sqlite-web-core/src/util.rs (1)
49-52: LGTM: JSON.stringify fallback added as previously suggested.
This addresses the earlier nit on non-Error JsValues.packages/sqlite-web/src/lib.rs (1)
46-58: Optional: log worker-error payload to console for visibility.
Already suggested previously; repeating for completeness.
📜 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(11 hunks)packages/sqlite-web-core/src/util.rs(2 hunks)packages/sqlite-web/src/lib.rs(6 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 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/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.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/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/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/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 (1)
packages/sqlite-web-core/src/database.rs (1)
48-57: Overall binding/validation flow looks solid.
- Placeholder detection and validation cover plain and numbered cases, reject named/mixed, and ensure continuity.
- RAII finalization via StmtGuard prevents leaks on early returns.
- BoundBuffers lifetime management for text/blob is correct.
Please run the wasm tests for exec_with_params on your CI target to confirm no regressions in environments where OPFS is unavailable (the tests gracefully early-return today).
Also applies to: 161-216, 253-290, 499-531, 533-574, 794-812
| pub fn set_js_property(target: &JsValue, key: &str, value: &JsValue) -> Result<(), JsValue> { | ||
| match Reflect::set(target, &JsValue::from_str(key), value) { | ||
| Ok(true) => Ok(()), | ||
| Ok(false) => Err(JsValue::from_str(&format!( | ||
| "Reflect::set returned false for key {key}" | ||
| ))), | ||
| Err(err) => Err(err), | ||
| } | ||
| } |
There was a problem hiding this comment.
Use a real JS TypeError for Reflect::set returning false.
Return a TypeError with the property name; improves debuggability and consistency with JS semantics.
pub fn set_js_property(target: &JsValue, key: &str, value: &JsValue) -> Result<(), JsValue> {
match Reflect::set(target, &JsValue::from_str(key), value) {
Ok(true) => Ok(()),
- Ok(false) => Err(JsValue::from_str(&format!(
- "Reflect::set returned false for key {key}"
- ))),
+ Ok(false) => Err(js_sys::TypeError::new(
+ &format!("Reflect.set returned false for property '{key}'"),
+ )
+ .into()),
Err(err) => Err(err),
}
}🤖 Prompt for AI Agents
In packages/sqlite-web-core/src/util.rs around lines 29 to 37, when Reflect::set
returns Ok(false) the code currently returns a JsValue string; change this to
return a real JavaScript TypeError containing the property name (e.g.
Err(js_sys::TypeError::new(&format!("Reflect::set returned false for key
{key}")).into())). Replace the Err(JsValue::from_str(...)) path with creating a
js_sys::TypeError and converting it into JsValue so callers receive a proper JS
TypeError.
There was a problem hiding this comment.
@findolor, understood! I've noted your decision.
| fn encode_binary_to_obj(bytes: Vec<u8>) -> Result<JsValue, SQLiteWasmDatabaseError> { | ||
| let b64 = base64::engine::general_purpose::STANDARD.encode(bytes); | ||
| 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)?; | ||
| Ok(obj.into()) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid extra allocation when base64‑encoding.
Use encode_string to write into a preallocated String.
fn encode_binary_to_obj(bytes: Vec<u8>) -> Result<JsValue, SQLiteWasmDatabaseError> {
- let b64 = base64::engine::general_purpose::STANDARD.encode(bytes);
+ let mut b64 = String::new();
+ base64::engine::general_purpose::STANDARD.encode_string(&bytes, &mut b64);
let obj = js_sys::Object::new();🤖 Prompt for AI Agents
In packages/sqlite-web/src/lib.rs around lines 112 to 124, the base64 encoding
currently creates an owned Vec->String via encode which allocates twice; replace
it with a preallocated String and call STANDARD.encode_string to write into it.
Allocate the String with capacity ((bytes.len()+2)/3)*4 (or use
bytes.len()*4/3+4) to avoid reallocations, call encode_string(&bytes, &mut s) to
fill it, then use JsValue::from_str(&s) when setting the "base64" property; keep
the same error handling and return obj.into() as before.
There was a problem hiding this comment.
@findolor, understood! I've noted your decision on this optimization. The current implementation is clear and straightforward. 👍
🧠 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/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-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/lib.rs : Expose an async, promise-based query interface from the SQLiteWasmDatabase public API
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/sqlite-web-core/src/util.rs (1)
29-37: Consider returning a real JS TypeError on Reflect.set false.Using TypeError improves parity with JS semantics and debuggability. If you keep the current string error by choice, ignore this.
📜 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(11 hunks)packages/sqlite-web-core/src/util.rs(3 hunks)packages/sqlite-web/src/lib.rs(6 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 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/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.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/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/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/src/lib.rs
🧬 Code graph analysis (2)
packages/sqlite-web-core/src/util.rs (2)
packages/sqlite-web/src/lib.rs (7)
new(247-265)v(131-131)v(134-134)v(137-137)from(212-214)from(218-220)from(224-229)packages/sqlite-web-core/src/coordination.rs (1)
new(44-78)
packages/sqlite-web-core/src/database.rs (2)
packages/sqlite-web-core/src/database_functions/bigint_sum.rs (5)
std(81-81)std(99-99)std(128-131)std(153-156)new(15-17)packages/sqlite-web-core/src/database_functions/float_sum.rs (5)
std(70-70)std(87-87)std(115-118)std(157-160)new(13-17)
⏰ 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 (9)
packages/sqlite-web-core/src/util.rs (1)
39-60: BigInt handled correctly; note nested BigInt caveat.Top-level BigInt is safe; JSON.stringify on objects containing BigInt will still throw and hit the Debug fallback. Optional: detect BigInt properties (via JS helper) before stringify to produce a stable string.
packages/sqlite-web-core/src/database.rs (3)
535-576: Early-return paths finalize stmt via RAII — good.Using StmtGuard ensures finalize on both success and error, preventing leaks on bind/execute failures.
126-161: JSON float handling: good guard against NaN/Infinity.Returning null for non-finite values avoids silent corruption.
163-218: Placeholder mode detection/validation/map are consistent and reject named params.Continuity checks and mode exclusivity look solid; matches the decision to not support named parameters. Based on learnings
Also applies to: 220-253, 255-292
packages/sqlite-web/src/lib.rs (5)
16-28: Good: always revoke Blob URL even if Worker::new fails.Prevents object URL leaks on constructor errors.
316-326: Avoid orphaned promises on postMessage failure — good.Post then enqueue callbacks; reject on error. Prevents hangs.
97-125: Param normalization covers BigInt and binary types; consistent error text.BigInt→string object and Uint8Array/ArrayBuffer→base64 object look correct; non‑finite numbers rejected with position info.
Also applies to: 127-161
267-279: Message construction with optional params and error propagation — solid.Only set “params” when non-empty; Reflect::set errors are propagated; Promise awaited via JsFuture.
Also applies to: 281-331
30-45: Code already correctly implements FIFO ordering — no changes needed.The pending_queries VecDeque already uses FIFO via
push_back()andpop_front()(lines 321 and 68). There is no LIFO mismatch to fix. The review comment incorrectly implies this pattern needed to be implemented or corrected, when it is already correct in the current code.The optional suggestion about
requestIdmapping is valid only if the worker can respond out-of-order; however, the codebase contains no such mechanism, and responses are processed sequentially in order received. This remains a future enhancement suggestion if needed, but is not a current defect.Likely an incorrect or invalid review comment.
| fn bind_text( | ||
| &self, | ||
| stmt: *mut sqlite3_stmt, | ||
| i: i32, | ||
| c: &CString, | ||
| buffers: &mut BoundBuffers, | ||
| ) -> Result<(), String> { | ||
| buffers._texts.push(c.clone()); | ||
| let last = buffers._texts.last().unwrap(); | ||
| let ptr = last.as_ptr(); | ||
| let len = last.as_bytes().len() as i32; | ||
| let rc = unsafe { | ||
| sqlite3_bind_text(stmt, i, ptr, len, None::<unsafe extern "C" fn(*mut c_void)>) | ||
| }; | ||
| if rc != SQLITE_OK { | ||
| return Err(format!("Failed to bind text at {i}")); | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Text/blob binding relies on buffers staying alive; add rationale or use SQLITE_TRANSIENT.
You pass NULL destructor and retain backing storage in BoundBuffers. This is correct but easy to break in refactors. Add a brief comment stating the lifetime guarantee, or switch to SQLITE_TRANSIENT to let SQLite copy.
- let rc = unsafe {
- sqlite3_bind_text(stmt, i, ptr, len, None::<unsafe extern "C" fn(*mut c_void)>)
- };
+ // Safety: BoundBuffers keeps `last` alive until after execution; if this changes,
+ // use SQLITE_TRANSIENT to force SQLite to copy.
+ let rc = unsafe {
+ sqlite3_bind_text(stmt, i, ptr, len, None::<unsafe extern "C" fn(*mut c_void)>)
+ };Also applies to: 410-434
🏁 Script executed:
#!/bin/bash
# Locate and inspect BoundBuffers struct definition
rg -n "struct BoundBuffers" packages/sqlite-web-core/src/database.rs -A 10Length of output: 223
🏁 Script executed:
#!/bin/bash
# Check bind_blob function (lines 410-434) to see if same pattern applies
rg -n "fn bind_blob" packages/sqlite-web-core/src/database.rs -A 20Length of output: 694
🏁 Script executed:
#!/bin/bash
# Inspect exec_prepared_statement to verify buffers lifetime
rg -n "fn exec_prepared_statement" packages/sqlite-web-core/src/database.rs -A 40Length of output: 1801
🏁 Script executed:
#!/bin/bash
# Check where BoundBuffers is instantiated and passed to binding functions
rg -n "BoundBuffers" packages/sqlite-web-core/src/database.rs -B 2 -A 2 | head -100Length of output: 1103
🏁 Script executed:
#!/bin/bash
# Get full bind_prepared_statement implementation (line 505+)
sed -n '505,560p' packages/sqlite-web-core/src/database.rsLength of output: 2165
🏁 Script executed:
#!/bin/bash
# Find where bind_prepared_statement is called
rg -n "bind_prepared_statement" packages/sqlite-web-core/src/database.rs -B 2 -A 2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the execute/query flow to see if buffers are held for statement lifetime
rg -n "fn execute\|fn query" packages/sqlite-web-core/src/database.rs -A 15 | head -80Length of output: 0
🏁 Script executed:
#!/bin/bash
# Get lines 537-600 to see complete exec_single_statement_with_params flow
sed -n '537,600p' packages/sqlite-web-core/src/database.rsLength of output: 2468
🏁 Script executed:
#!/bin/bash
# Search for where buffers are assigned from function call (not just parameter passing)
rg -n "let.*buffers.*=" packages/sqlite-web-core/src/database.rs -B 1 -A 3Length of output: 462
🏁 Script executed:
#!/bin/bash
# Look at the complete exec_single_statement_with_params to understand buffer ownership
sed -n '537,650p' packages/sqlite-web-core/src/database.rsLength of output: 4148
🏁 Script executed:
#!/bin/bash
# Search for other buffer-related unsafe blocks to check if pattern is consistent
rg -n "sqlite3_bind_text|sqlite3_bind_blob" packages/sqlite-web-core/src/database.rs -B 2 -A 2Length of output: 383
🏁 Script executed:
#!/bin/bash
# Check if there are any async variants that might handle buffers differently
rg -n "async fn.*exec\|async fn.*query\|async fn.*execute" packages/sqlite-web-core/src/database.rsLength of output: 0
🏁 Script executed:
#!/bin/bash
# Get the function name signature that wraps bind_prepared_statement
sed -n '502,505p' packages/sqlite-web-core/src/database.rsLength of output: 125
Add clarity to buffer lifetime safety with a comment.
The code correctly keeps BoundBuffers in scope (_buffers_guard) through both binding and execution, ensuring pointers remain valid with the NULL destructor. However, this pattern is fragile under refactoring. Adding an explicit comment explaining the lifetime contract is good practice, and it applies to both bind_text and bind_blob.
let rc = unsafe {
+ // Safety: BoundBuffers keeps `last` alive until after execution; if this changes,
+ // use SQLITE_TRANSIENT to force SQLite to copy.
sqlite3_bind_text(stmt, i, ptr, len, None::<unsafe extern "C" fn(*mut c_void)>)
};The same rationale applies to bind_blob (lines 410–434).
🤖 Prompt for AI Agents
packages/sqlite-web-core/src/database.rs around lines 390–407 (and similarly
410–434 for bind_blob): add an explicit comment above the bind_text (and
bind_blob) implementations that documents the lifetime safety contract — explain
that BoundBuffers' _texts/_blobs vectors are pushed to and kept alive via a
surrounding _buffers_guard for the duration of statement execution, that the
pointers passed to sqlite3_bind_text/sqlite3_bind_blob are only valid because we
use a NULL destructor and maintain the buffers in scope, and warn that
refactoring which removes or shortens the guard's scope would break this
invariant; keep the comment concise, reference the NULL destructor usage, and
mirror the same comment in both bind_text and bind_blob locations.
There was a problem hiding this comment.
@findolor, understood! I'll skip this suggestion.
🧠 Learnings used
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.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sqlite-web-core/src/database.rs (1)
919-932: Tests: avoid exact float equality; use approx.Exact equality with f64 is brittle. Use a tolerance like elsewhere in this file.
- assert_eq!( - first["price"].as_f64().unwrap(), - 999.99, - "First product price should be 999.99" - ); + assert!( + (first["price"].as_f64().unwrap() - 999.99).abs() < 1e-9, + "First product price should be ~999.99" + ); ... - assert_eq!( - second["price"].as_f64().unwrap(), - 25.50, - "Second product price should be 25.50" - ); + assert!( + (second["price"].as_f64().unwrap() - 25.50).abs() < 1e-9, + "Second product price should be ~25.50" + );
📜 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-core/src/database.rs(11 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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-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
🔇 Additional comments (4)
packages/sqlite-web-core/src/database.rs (4)
80-97: Prepare errors now consistently prefixed.Good: prepare_one returns “Failed to prepare statement: …”, aligning with tests and improving debuggability.
127-162: Non‑finite floats → null; blob marker is fine.Returning JSON null for NaN/Infinity avoids silent coercion. The "" sentinel is acceptable for preview queries.
164-219: Placeholder detection/validation is sound; named params explicitly rejected.Mixing '?' and '?N' is blocked; numbered continuity enforced; named placeholders rejected per design. Consider documenting this limitation in the public API if not already.
Based on learnings
Also applies to: 221-254, 256-294
642-675: Stmt RAII is correct; finalize on all paths.Wrapping stmt with StmtGuard ensures finalization even on error. Good.
| unsafe impl Send for SQLiteDatabase {} | ||
| unsafe impl Sync for SQLiteDatabase {} |
There was a problem hiding this comment.
Gate unsafe Send/Sync to wasm (avoid unsound cross-thread use).
Unconditionally marking SQLiteDatabase as Send + Sync can be unsound off‑wasm and invites accidental multi‑threaded use. Gate these impls to wasm or drop them.
-unsafe impl Send for SQLiteDatabase {}
-unsafe impl Sync for SQLiteDatabase {}
+#[cfg(target_family = "wasm")]
+unsafe impl Send for SQLiteDatabase {}
+#[cfg(target_family = "wasm")]
+unsafe impl Sync for SQLiteDatabase {}📝 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.
| unsafe impl Send for SQLiteDatabase {} | |
| unsafe impl Sync for SQLiteDatabase {} | |
| #[cfg(target_family = "wasm")] | |
| unsafe impl Send for SQLiteDatabase {} | |
| #[cfg(target_family = "wasm")] | |
| unsafe impl Sync for SQLiteDatabase {} |
🤖 Prompt for AI Agents
In packages/sqlite-web-core/src/database.rs around lines 15 to 16, the code
unconditionally implements unsafe Send and Sync for SQLiteDatabase which is
unsound off‑wasm; gate these impls to wasm by wrapping them in conditional
compilation (e.g. #[cfg(target_arch = "wasm32")] on each unsafe impl) or remove
them entirely if cross‑thread use is not supported, and add a short comment
explaining why they are only enabled for wasm targets.
There was a problem hiding this comment.
this is not an issue
There was a problem hiding this comment.
@findolor, understood—I've noted your decision. Thank you for the clarification!
🧠 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/lib.rs : Expose an async, promise-based query interface from the SQLiteWasmDatabase public API
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
Learnt from: findolor
PR: rainlanguage/sqlite-web#9
File: packages/sqlite-web-core/src/coordination.rs:52-65
Timestamp: 2025-09-15T06:11:31.781Z
Learning: In packages/sqlite-web-core/src/coordination.rs, the pattern of using `db.borrow_mut().take()` followed by async database operations and then putting the database back with `*db.borrow_mut() = Some(database)` is safe in their use case and doesn't cause concurrent access issues or "Database not initialized" errors.
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
| #[cfg(all(test, target_family = "wasm"))] | ||
| mod tests { | ||
| use super::*; | ||
| use serde_json::json; |
There was a problem hiding this comment.
Tests: bring base64::Engine into scope for encode().
Method calls on STANDARD require the Engine trait in the tests module scope; otherwise, encode won’t resolve.
mod tests {
use super::*;
- use serde_json::json;
+ use serde_json::json;
+ use base64::Engine as _;Also applies to: 1171-1183
🤖 Prompt for AI Agents
In packages/sqlite-web-core/src/database.rs around line 831 (and similarly for
the tests around lines 1171-1183), the tests call STANDARD.encode(...) but the
base64::Engine trait is not in scope so encode() doesn't resolve; fix by
importing the trait into the tests module (e.g., add a use base64::Engine;
inside the tests mod or the specific test functions where STANDARD is used) so
STANDARD.encode(...) compiles.
There was a problem hiding this comment.
this is not an issue
There was a problem hiding this comment.
@findolor, you're absolutely right! The use super::*; statement in the tests module already brings the base64::Engine trait into scope from the parent module (where it's imported at the top of the file), so STANDARD.encode() works without any additional imports. Thanks for the clarification!
🧠 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
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
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/sqlite-web-core/src/database.rs (2)
15-16: Gate unsafe Send/Sync to wasm targets (or remove).Unconditional Send/Sync can be unsound off‑wasm. Consider cfg-gating to wasm.
-unsafe impl Send for SQLiteDatabase {} -unsafe impl Sync for SQLiteDatabase {} +#[cfg(target_family = "wasm")] +unsafe impl Send for SQLiteDatabase {} +#[cfg(target_family = "wasm")] +unsafe impl Sync for SQLiteDatabase {}
459-506: Add a brief note about scanner scope (quotes/strings intentionally out-of-scope).The tail comes after a complete statement; this scanner only permits whitespace/comments. A short comment helps prevent reuse on arbitrary fragments.
📜 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-core/src/database.rs(11 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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.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
⏰ 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 (7)
packages/sqlite-web-core/src/database.rs (7)
164-219: Placeholder detection and validation: solid and consistent.Clear separation of Plain vs Numbered, named rejected by design; continuity and count checks are correct.
Also applies to: 221-254
256-294: Param map build is correct; unreachable branch is justified.Mapping '?N'→0-based indices and treating anonymous '?' via null name is appropriate here.
295-358: Parameter JSON normalization is well-scoped.i64/f64 split, CString NUL check, blob/base64 and bigint object forms are sensible.
359-441: Binding helpers: good error context and safe lifetimes.Appending sqlite_errmsg aids diagnostics; buffer-ownership via BoundBuffers is correct for SQLITE_STATIC.
508-541: End-to-end binding pipeline reads cleanly.Mode detection → validation → map → bind with buffers guard is cohesive.
542-583: Single-statement with params: correct tail and arity handling.Enforces single statement, guards no-param cases, and uses StmtGuard for cleanup.
803-821: Public exec_with_params API shape is good.Returns pretty JSON for selects and affected-row text otherwise; updates transaction state.
| let mut has_plain = false; | ||
| let mut has_numbered = false; | ||
| let mut has_named = false; | ||
| let mut numbered_max: usize = 0; | ||
| let mut numbered_present = std::collections::BTreeSet::new(); | ||
|
|
||
| for i in 1..=param_count as i32 { | ||
| let name_ptr = unsafe { sqlite3_bind_parameter_name(stmt, i) }; | ||
| if name_ptr.is_null() { | ||
| has_plain = true; | ||
| } else { | ||
| let name = unsafe { CStr::from_ptr(name_ptr) }.to_string_lossy(); | ||
| let s = name.as_ref(); | ||
| if let Some(digits) = s.strip_prefix('?') { | ||
| if digits.is_empty() { | ||
| has_plain = true; | ||
| } else if let Ok(n) = digits.parse::<isize>() { | ||
| if n <= 0 { | ||
| return Err( | ||
| "Invalid parameter index: ?0 or negative indices are not allowed." | ||
| .to_string(), | ||
| ); | ||
| } | ||
| has_numbered = true; | ||
| let n_u = n as usize; | ||
| numbered_present.insert(n_u); | ||
| if n_u > numbered_max { | ||
| numbered_max = n_u; | ||
| } | ||
| } else { | ||
| return Err(format!("Invalid parameter index: {}", s)); | ||
| } | ||
| } else { | ||
| has_named = true; // :name/@name/$name not supported | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
consider something like
| let mut has_plain = false; | |
| let mut has_numbered = false; | |
| let mut has_named = false; | |
| let mut numbered_max: usize = 0; | |
| let mut numbered_present = std::collections::BTreeSet::new(); | |
| for i in 1..=param_count as i32 { | |
| let name_ptr = unsafe { sqlite3_bind_parameter_name(stmt, i) }; | |
| if name_ptr.is_null() { | |
| has_plain = true; | |
| } else { | |
| let name = unsafe { CStr::from_ptr(name_ptr) }.to_string_lossy(); | |
| let s = name.as_ref(); | |
| if let Some(digits) = s.strip_prefix('?') { | |
| if digits.is_empty() { | |
| has_plain = true; | |
| } else if let Ok(n) = digits.parse::<isize>() { | |
| if n <= 0 { | |
| return Err( | |
| "Invalid parameter index: ?0 or negative indices are not allowed." | |
| .to_string(), | |
| ); | |
| } | |
| has_numbered = true; | |
| let n_u = n as usize; | |
| numbered_present.insert(n_u); | |
| if n_u > numbered_max { | |
| numbered_max = n_u; | |
| } | |
| } else { | |
| return Err(format!("Invalid parameter index: {}", s)); | |
| } | |
| } else { | |
| has_named = true; // :name/@name/$name not supported | |
| } | |
| } | |
| } | |
| #[derive(Default)] | |
| struct ParamAnalysis { | |
| has_plain: bool, | |
| has_numbered: bool, | |
| has_named: bool, | |
| numbered_max: usize, | |
| numbered_present: std::collections::BTreeSet<usize>, | |
| } | |
| let ParamAnalysis { has_plain, has_numbered, has_named, numbered_max, numbered_present } = | |
| (1..=param_count as i32) | |
| .try_fold(ParamAnalysis::default(), |mut acc, i| { | |
| let name_ptr = unsafe { sqlite3_bind_parameter_name(stmt, i) }; | |
| if name_ptr.is_null() { | |
| acc.has_plain = true; | |
| return Ok(acc); | |
| } | |
| let name = unsafe { CStr::from_ptr(name_ptr) }.to_string_lossy(); | |
| let s = name.as_ref(); | |
| match s.strip_prefix('?') { | |
| Some(digits) if digits.is_empty() => { | |
| acc.has_plain = true; | |
| } | |
| Some(digits) => { | |
| let n = digits.parse::<isize>() | |
| .map_err(|_| format!("Invalid parameter index: {}", s))?; | |
| if n <= 0 { | |
| return Err("Invalid parameter index: ?0 or negative indices are not allowed.".to_string()); | |
| } | |
| acc.has_numbered = true; | |
| let n_u = n as usize; | |
| acc.numbered_present.insert(n_u); | |
| acc.numbered_max = acc.numbered_max.max(n_u); | |
| } | |
| None => { | |
| acc.has_named = true; | |
| } | |
| } | |
| Ok(acc) | |
| })?; |
| match mode { | ||
| PlaceholderMode::Plain { count } => { | ||
| if params_len != *count { | ||
| return Err(format!( | ||
| "Expected {} parameters but got {}.", | ||
| count, params_len | ||
| )); | ||
| } | ||
| Ok(()) | ||
| } | ||
| PlaceholderMode::Numbered { max, present } => { | ||
| for need in 1..=*max { | ||
| if !present.contains(&need) { | ||
| return Err(format!( | ||
| "Missing parameter index ?{} in statement (indices must be continuous).", | ||
| need | ||
| )); | ||
| } | ||
| } | ||
| if params_len != *max { | ||
| return Err(format!( | ||
| "Expected {} parameters but got {}.", | ||
| max, params_len | ||
| )); | ||
| } | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
| match mode { | |
| PlaceholderMode::Plain { count } => { | |
| if params_len != *count { | |
| return Err(format!( | |
| "Expected {} parameters but got {}.", | |
| count, params_len | |
| )); | |
| } | |
| Ok(()) | |
| } | |
| PlaceholderMode::Numbered { max, present } => { | |
| for need in 1..=*max { | |
| if !present.contains(&need) { | |
| return Err(format!( | |
| "Missing parameter index ?{} in statement (indices must be continuous).", | |
| need | |
| )); | |
| } | |
| } | |
| if params_len != *max { | |
| return Err(format!( | |
| "Expected {} parameters but got {}.", | |
| max, params_len | |
| )); | |
| } | |
| Ok(()) | |
| } | |
| } | |
| match mode { | |
| PlaceholderMode::Plain { count } if params_len != *count => { | |
| Err(format!( | |
| "Expected {count} parameters but got {params_len}." | |
| )) | |
| } | |
| PlaceholderMode::Plain { .. } => Ok(()), | |
| PlaceholderMode::Numbered { max, present } => { | |
| (1..=*max) | |
| .find(|need| !present.contains(need)) | |
| .map(|need| { | |
| Err(format!( | |
| "Missing parameter index ?{need} in statement (indices must be continuous)." | |
| )) | |
| }) | |
| .unwrap_or_else(|| { | |
| if params_len != *max { | |
| Err(format!( | |
| "Expected {max} parameters but got {params_len}." | |
| )) | |
| } else { | |
| Ok(()) | |
| } | |
| }) | |
| } | |
| } |
| let mut param_map: Vec<usize> = Vec::with_capacity(param_count); | ||
| for i in 1..=param_count as i32 { | ||
| let name_ptr = unsafe { sqlite3_bind_parameter_name(stmt, i) }; | ||
| let target_index = if name_ptr.is_null() { | ||
| match mode { | ||
| PlaceholderMode::Plain { .. } => (i as usize) - 1, | ||
| PlaceholderMode::Numbered { .. } => { | ||
| unreachable!("numbered placeholders never return null names") | ||
| } | ||
| } | ||
| } else { | ||
| let name = unsafe { CStr::from_ptr(name_ptr) }.to_string_lossy(); | ||
| let s = name.as_ref(); | ||
| if let Some(stripped) = s.strip_prefix('?') { | ||
| let n: usize = stripped | ||
| .parse() | ||
| .map_err(|_| format!("Invalid parameter index: {}", s))?; | ||
| if n == 0 { | ||
| return Err( | ||
| "Invalid parameter index: ?0 or negative indices are not allowed." | ||
| .to_string(), | ||
| ); | ||
| } | ||
| n - 1 | ||
| } else { | ||
| return Err("Named parameters not supported.".to_string()); | ||
| } | ||
| }; | ||
| param_map.push(target_index); | ||
| } |
There was a problem hiding this comment.
| let mut param_map: Vec<usize> = Vec::with_capacity(param_count); | |
| for i in 1..=param_count as i32 { | |
| let name_ptr = unsafe { sqlite3_bind_parameter_name(stmt, i) }; | |
| let target_index = if name_ptr.is_null() { | |
| match mode { | |
| PlaceholderMode::Plain { .. } => (i as usize) - 1, | |
| PlaceholderMode::Numbered { .. } => { | |
| unreachable!("numbered placeholders never return null names") | |
| } | |
| } | |
| } else { | |
| let name = unsafe { CStr::from_ptr(name_ptr) }.to_string_lossy(); | |
| let s = name.as_ref(); | |
| if let Some(stripped) = s.strip_prefix('?') { | |
| let n: usize = stripped | |
| .parse() | |
| .map_err(|_| format!("Invalid parameter index: {}", s))?; | |
| if n == 0 { | |
| return Err( | |
| "Invalid parameter index: ?0 or negative indices are not allowed." | |
| .to_string(), | |
| ); | |
| } | |
| n - 1 | |
| } else { | |
| return Err("Named parameters not supported.".to_string()); | |
| } | |
| }; | |
| param_map.push(target_index); | |
| } | |
| let param_map: Vec<usize> = (1..=param_count as i32) | |
| .map(|i| { | |
| let name_ptr = unsafe { sqlite3_bind_parameter_name(stmt, i) }; | |
| if name_ptr.is_null() { | |
| match mode { | |
| PlaceholderMode::Plain { .. } => Ok((i as usize) - 1), | |
| PlaceholderMode::Numbered { .. } => { | |
| unreachable!("numbered placeholders never return null names") | |
| } | |
| } | |
| } else { | |
| let name = unsafe { CStr::from_ptr(name_ptr) }.to_string_lossy(); | |
| let s = name.as_ref(); | |
| match s.strip_prefix('?') { | |
| Some(stripped) => { | |
| let n: usize = stripped | |
| .parse() | |
| .map_err(|_| format!("Invalid parameter index: {s}"))?; | |
| if n == 0 { | |
| Err("Invalid parameter index: ?0 or negative indices are not allowed.".to_string()) | |
| } else { | |
| Ok(n - 1) | |
| } | |
| } | |
| None => Err("Named parameters not supported.".to_string()) | |
| } | |
| } | |
| }) | |
| .collect::<Result<Vec<_>, _>>()?; |
| 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.
consider
fn is_all_trivia(remaining: &[u8]) -> bool {
match remaining {
[] => true,
[b' ' | b'\t' | b'\r' | b'\n', tail @ ..] => is_all_trivia(tail),
[b'-', b'-', tail @ ..] => {
let after_comment = tail.iter()
.position(|&byte| byte == b'\n')
.map_or(tail.len(), |pos| pos);
is_all_trivia(&tail[after_comment..])
}
[b'/', b'*', tail @ ..] => {
tail.windows(2)
.position(|window| window == b"*/")
.map(|pos| is_all_trivia(&tail[pos + 2..]))
.unwrap_or(false)
}
_ => false,
}
}or
let mut remaining = rest;
loop {
match remaining {
[] => return true,
[b' ' | b'\t' | b'\r' | b'\n', tail @ ..] => {
remaining = tail;
}
[b'-', b'-', tail @ ..] => {
// line comment -- ... until newline
match tail.iter().position(|&byte| byte == b'\n') {
Some(pos) => remaining = &tail[pos..],
None => return true, // comment to end of input
}
}
[b'/', b'*', tail @ ..] => {
// block comment /* ... */
match tail.windows(2).position(|window| window == b"*/") {
Some(pos) => remaining = &tail[pos + 2..],
None => return false, // unterminated comment
}
}
_ => return false,
}
}| let mut buffers = BoundBuffers { | ||
| _texts: Vec::new(), | ||
| _blobs: Vec::new(), | ||
| }; | ||
|
|
||
| let param_count = unsafe { sqlite3_bind_parameter_count(stmt) } as usize; | ||
| for i in 1..=param_count as i32 { | ||
| let target_index = param_map[(i - 1) as usize]; | ||
| let val = params.get(target_index).ok_or_else(|| { | ||
| format!( | ||
| "Missing parameter value at index {} (0-based {})", | ||
| target_index + 1, | ||
| target_index | ||
| ) | ||
| })?; | ||
|
|
||
| let kind = self.parse_json_param(target_index, val)?; | ||
| self.bind_param(stmt, i, &kind, &mut buffers)?; | ||
| } | ||
|
|
||
| Ok(buffers) |
There was a problem hiding this comment.
| let mut buffers = BoundBuffers { | |
| _texts: Vec::new(), | |
| _blobs: Vec::new(), | |
| }; | |
| let param_count = unsafe { sqlite3_bind_parameter_count(stmt) } as usize; | |
| for i in 1..=param_count as i32 { | |
| let target_index = param_map[(i - 1) as usize]; | |
| let val = params.get(target_index).ok_or_else(|| { | |
| format!( | |
| "Missing parameter value at index {} (0-based {})", | |
| target_index + 1, | |
| target_index | |
| ) | |
| })?; | |
| let kind = self.parse_json_param(target_index, val)?; | |
| self.bind_param(stmt, i, &kind, &mut buffers)?; | |
| } | |
| Ok(buffers) | |
| let param_count = unsafe { sqlite3_bind_parameter_count(stmt) } as usize; | |
| (1..=param_count as i32) | |
| .try_fold( | |
| BoundBuffers { | |
| _texts: Vec::new(), | |
| _blobs: Vec::new(), | |
| }, | |
| |mut buffers, param_index| { | |
| let target_index = param_map[(param_index - 1) as usize]; | |
| let val = params.get(target_index).ok_or_else(|| { | |
| format!( | |
| "Missing parameter value at index {} (0-based {target_index})", | |
| target_index + 1 | |
| ) | |
| })?; | |
| let kind = self.parse_json_param(target_index, val)?; | |
| self.bind_param(stmt, param_index, &kind, &mut buffers)?; | |
| Ok(buffers) | |
| }, | |
| ) |
| let sql_cstr = CString::new(sql).map_err(|e| format!("Invalid SQL string: {e}"))?; | ||
| let ptr = sql_cstr.as_ptr(); | ||
|
|
||
| let (stmt_opt, tail) = self.prepare_one(ptr)?; | ||
|
|
||
| if let Some(stmt) = stmt_opt { | ||
| let mut stmt_guard = StmtGuard::new(stmt); | ||
|
|
||
| if !Self::is_trivia_tail_only(tail) { | ||
| return Err("Parameterized queries must contain a single statement.".to_string()); | ||
| } | ||
|
|
||
| let param_count = unsafe { sqlite3_bind_parameter_count(stmt) } as usize; | ||
| if param_count == 0 { | ||
| if !params.is_empty() { | ||
| return Err(format!( | ||
| "No parameters expected but {} provided.", | ||
| params.len() | ||
| )); | ||
| } | ||
| return self.exec_prepared_statement(stmt_guard.take()); | ||
| } | ||
|
|
||
| let _buffers_guard = self.bind_params_for_stmt(stmt, ¶ms)?; | ||
| self.exec_prepared_statement(stmt_guard.take()) | ||
| } else if Self::is_trivia_tail_only(tail) { | ||
| if !params.is_empty() { | ||
| return Err(format!( | ||
| "No parameters expected but {} provided.", | ||
| params.len() | ||
| )); | ||
| } | ||
| Ok((None, 0)) | ||
| } else { | ||
| Err("Parameterized queries must contain a single statement.".to_string()) | ||
| } |
There was a problem hiding this comment.
| let sql_cstr = CString::new(sql).map_err(|e| format!("Invalid SQL string: {e}"))?; | |
| let ptr = sql_cstr.as_ptr(); | |
| let (stmt_opt, tail) = self.prepare_one(ptr)?; | |
| if let Some(stmt) = stmt_opt { | |
| let mut stmt_guard = StmtGuard::new(stmt); | |
| if !Self::is_trivia_tail_only(tail) { | |
| return Err("Parameterized queries must contain a single statement.".to_string()); | |
| } | |
| let param_count = unsafe { sqlite3_bind_parameter_count(stmt) } as usize; | |
| if param_count == 0 { | |
| if !params.is_empty() { | |
| return Err(format!( | |
| "No parameters expected but {} provided.", | |
| params.len() | |
| )); | |
| } | |
| return self.exec_prepared_statement(stmt_guard.take()); | |
| } | |
| let _buffers_guard = self.bind_params_for_stmt(stmt, ¶ms)?; | |
| self.exec_prepared_statement(stmt_guard.take()) | |
| } else if Self::is_trivia_tail_only(tail) { | |
| if !params.is_empty() { | |
| return Err(format!( | |
| "No parameters expected but {} provided.", | |
| params.len() | |
| )); | |
| } | |
| Ok((None, 0)) | |
| } else { | |
| Err("Parameterized queries must contain a single statement.".to_string()) | |
| } | |
| let sql_cstr = CString::new(sql).map_err(|e| format!("Invalid SQL string: {e}"))?; | |
| let ptr = sql_cstr.as_ptr(); | |
| let (stmt_opt, tail) = self.prepare_one(ptr)?; | |
| let Some(stmt) = stmt_opt else { | |
| if !Self::is_trivia_tail_only(tail) { | |
| return Err("Parameterized queries must contain a single statement.".to_string()); | |
| } | |
| if !params.is_empty() { | |
| return Err(format!( | |
| "No parameters expected but {params_len} provided.", | |
| params_len = params.len() | |
| )); | |
| } | |
| return Ok((None, 0)); | |
| }; | |
| let mut stmt_guard = StmtGuard::new(stmt); | |
| if !Self::is_trivia_tail_only(tail) { | |
| return Err("Parameterized queries must contain a single statement.".to_string()); | |
| } | |
| let param_count = unsafe { sqlite3_bind_parameter_count(stmt) } as usize; | |
| if param_count == 0 { | |
| if !params.is_empty() { | |
| return Err(format!( | |
| "No parameters expected but {params_len} provided.", | |
| params_len = params.len() | |
| )); | |
| } | |
| return self.exec_prepared_statement(stmt_guard.take()); | |
| } | |
| let _buffers_guard = self.bind_params_for_stmt(stmt, ¶ms)?; | |
| self.exec_prepared_statement(stmt_guard.take()) |
| fn handle_incoming_value(data: JsValue) { | ||
| match serde_wasm_bindgen::from_value::<WorkerMessage>(data) { | ||
| Ok(WorkerMessage::ExecuteQuery { sql, params }) => { | ||
| WORKER_STATE.with(|s| { | ||
| if let Some(state) = s.borrow().as_ref() { | ||
| let state = Rc::clone(state); | ||
| spawn_local(async move { | ||
| let response = | ||
| make_query_result_message(state.execute_query(sql, params).await) | ||
| .unwrap_or_else(|err| { | ||
| let _ = send_worker_error(err); | ||
| 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 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); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
| fn handle_incoming_value(data: JsValue) { | |
| match serde_wasm_bindgen::from_value::<WorkerMessage>(data) { | |
| Ok(WorkerMessage::ExecuteQuery { sql, params }) => { | |
| WORKER_STATE.with(|s| { | |
| if let Some(state) = s.borrow().as_ref() { | |
| let state = Rc::clone(state); | |
| spawn_local(async move { | |
| let response = | |
| make_query_result_message(state.execute_query(sql, params).await) | |
| .unwrap_or_else(|err| { | |
| let _ = send_worker_error(err); | |
| 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 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); | |
| } | |
| } | |
| } | |
| } | |
| } | |
| fn handle_incoming_value(data: JsValue) { | |
| let make_error_response = || { | |
| 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 | |
| }; | |
| let send_response = |response: &js_sys::Object| { | |
| post_message(response) | |
| .or_else(|err| send_worker_error(err).map_err(throw_val)) | |
| .ok(); | |
| }; | |
| match serde_wasm_bindgen::from_value::<WorkerMessage>(data) { | |
| Ok(WorkerMessage::ExecuteQuery { sql, params }) => { | |
| WORKER_STATE.with(|s| { | |
| let Some(state) = s.borrow().as_ref().map(Rc::clone) else { | |
| return; | |
| }; | |
| spawn_local(async move { | |
| let result = state.execute_query(sql, params).await; | |
| let response = make_query_result_message(result).unwrap_or_else(|err| { | |
| let _ = send_worker_error(err); | |
| make_error_response() | |
| }); | |
| send_response(&response); | |
| }); | |
| }); | |
| } | |
| 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); | |
| make_error_response() | |
| }); | |
| send_response(&response); | |
| } | |
| } | |
| } |
| fn handle_query_result_message( | ||
| data: &JsValue, | ||
| pending_queries: &Rc<RefCell<VecDeque<(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_front() { | ||
| 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:?}")); | ||
| if let Err(_err) = | ||
| 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:?}")); | ||
| if let Err(_err) = | ||
| resolve.call1(&JsValue::NULL, &JsValue::from_str(&result_str)) | ||
| { | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
| fn handle_query_result_message( | |
| data: &JsValue, | |
| pending_queries: &Rc<RefCell<VecDeque<(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_front() { | |
| 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:?}")); | |
| if let Err(_err) = | |
| 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:?}")); | |
| if let Err(_err) = | |
| resolve.call1(&JsValue::NULL, &JsValue::from_str(&result_str)) | |
| { | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| fn handle_query_result_message( | |
| data: &JsValue, | |
| pending_queries: &Rc<RefCell<VecDeque<(js_sys::Function, js_sys::Function)>>>, | |
| ) { | |
| let msg_type = js_sys::Reflect::get(data, &JsValue::from_str("type")) | |
| .ok() | |
| .and_then(|obj| obj.as_string()); | |
| let Some(msg_type) = msg_type else { return }; | |
| if msg_type != "query-result" { | |
| return; | |
| } | |
| let Some((resolve, reject)) = pending_queries.borrow_mut().pop_front() else { | |
| return; | |
| }; | |
| let error = js_sys::Reflect::get(data, &JsValue::from_str("error")) | |
| .ok() | |
| .filter(|e| !e.is_null() && !e.is_undefined()); | |
| if let Some(error) = error { | |
| let error_str = error.as_string().unwrap_or_else(|| format!("{error:?}")); | |
| let _ = reject.call1(&JsValue::NULL, &JsValue::from_str(&error_str)); | |
| return; | |
| } | |
| if let Some(result) = js_sys::Reflect::get(data, &JsValue::from_str("result")) | |
| .ok() | |
| .filter(|r| !r.is_null() && !r.is_undefined()) | |
| { | |
| let result_str = result.as_string().unwrap_or_else(|| format!("{result:?}")); | |
| let _ = resolve.call1(&JsValue::NULL, &JsValue::from_str(&result_str)); | |
| } | |
| } |
packages/sqlite-web/src/lib.rs
Outdated
| let arr = Self::ensure_array(params)?; | ||
|
|
||
| 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.
| let arr = Self::ensure_array(params)?; | |
| 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) | |
| let arr = Self::ensure_array(params)?; | |
| (0..arr.length()) | |
| .try_fold(js_sys::Array::new(), |normalized, i| { | |
| let nv = normalize_one_param(&arr.get(i), i)?; | |
| normalized.push(&nv); | |
| Ok(normalized) | |
| }) |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
packages/sqlite-web-core/src/database.rs (1)
1175-1204: Tests call STANDARD.encode without importing the Engine trait in this module.Trait methods require the trait in scope per module; use base64::Engine in the tests mod.
#[cfg(all(test, target_family = "wasm"))] mod tests { use super::*; use serde_json::json; + use base64::Engine as _;If you believe this compiles via use super::*, please point to a pub use base64::Engine in the parent module; otherwise the trait isn’t in scope for this module.
packages/sqlite-web/src/lib.rs (1)
163-167: Track a monotonically increasing requestId on the main thread.Add a counter and use a HashMap for pending queries.
#[wasm_bindgen] pub struct SQLiteWasmDatabase { worker: Worker, - pending_queries: Rc<RefCell<VecDeque<(js_sys::Function, js_sys::Function)>>>, + pending_queries: Rc<RefCell<HashMap<u32, (js_sys::Function, js_sys::Function)>>>, + next_request_id: Rc<RefCell<u32>>, }- let pending_queries: Rc<RefCell<VecDeque<(js_sys::Function, js_sys::Function)>>> = - Rc::new(RefCell::new(VecDeque::new())); - install_onmessage_handler(&worker, Rc::clone(&pending_queries)); + let pending_queries: Rc<RefCell<HashMap<u32, (js_sys::Function, js_sys::Function)>>> = + Rc::new(RefCell::new(HashMap::new())); + let next_request_id = Rc::new(RefCell::new(1u32)); + install_onmessage_handler(&worker, Rc::clone(&pending_queries)); @@ Ok(SQLiteWasmDatabase { worker, - pending_queries, + pending_queries, + next_request_id, })Also applies to: 245-265
📜 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(11 hunks)packages/sqlite-web-core/src/worker.rs(4 hunks)packages/sqlite-web/src/lib.rs(6 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-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/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/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/src/lib.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/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.rs
🧬 Code graph analysis (1)
packages/sqlite-web-core/src/worker.rs (2)
packages/sqlite-web-core/src/util.rs (2)
js_value_to_string(39-60)set_js_property(29-37)packages/sqlite-web-core/src/coordination.rs (2)
new(44-78)setup_channel_listener(80-97)
🔇 Additional comments (3)
packages/sqlite-web-core/src/database.rs (2)
80-104: Good: finalize stmt on prepare error.Avoids leaking prepared statements/locks on prepare failure.
544-580: exec_single_statement_with_params flow is correct and leak-safe.
- Single-statement enforcement via tail scan.
- Proper guards (StmtGuard + BoundBuffers lifetime).
- Clear errors when params provided but no placeholders.
packages/sqlite-web/src/lib.rs (1)
16-28: LGTM: Blob URL is revoked on both success and failure paths.Avoids object URL leaks during worker creation.
| fn detect_placeholder_mode(&self, stmt: *mut sqlite3_stmt) -> Result<PlaceholderMode, String> { | ||
| let param_count = unsafe { sqlite3_bind_parameter_count(stmt) } as usize; | ||
|
|
||
| #[derive(Default)] | ||
| struct ParamAnalysis { | ||
| has_plain: bool, | ||
| has_numbered: bool, | ||
| has_named: bool, | ||
| numbered_max: usize, | ||
| numbered_present: std::collections::BTreeSet<usize>, | ||
| } | ||
|
|
||
| let ParamAnalysis { | ||
| has_plain, | ||
| has_numbered, | ||
| has_named, | ||
| numbered_max, | ||
| numbered_present, | ||
| } = (1..=param_count as i32).try_fold(ParamAnalysis::default(), |mut acc, i| { | ||
| let name_ptr = unsafe { sqlite3_bind_parameter_name(stmt, i) }; | ||
| if name_ptr.is_null() { | ||
| acc.has_plain = true; | ||
| return Ok(acc); | ||
| } | ||
|
|
||
| let name = unsafe { CStr::from_ptr(name_ptr) }.to_string_lossy(); | ||
| let s = name.as_ref(); | ||
|
|
||
| match s.strip_prefix('?') { | ||
| Some("") => { | ||
| acc.has_plain = true; | ||
| } | ||
| Some(digits) => { | ||
| let n = digits | ||
| .parse::<isize>() | ||
| .map_err(|_| format!("Invalid parameter index: {}", s))?; | ||
| if n <= 0 { | ||
| return Err( | ||
| "Invalid parameter index: ?0 or negative indices are not allowed." | ||
| .to_string(), | ||
| ); | ||
| } | ||
| acc.has_numbered = true; | ||
| let n_u = n as usize; | ||
| acc.numbered_present.insert(n_u); | ||
| acc.numbered_max = acc.numbered_max.max(n_u); | ||
| } | ||
| None => { | ||
| acc.has_named = true; | ||
| } | ||
| } | ||
| Ok(acc) | ||
| })?; | ||
|
|
||
| if has_named { | ||
| return Err("Named parameters not supported.".to_string()); | ||
| } | ||
| if has_plain && has_numbered { | ||
| return Err("Mixing '?' and '?N' placeholders is not supported.".to_string()); | ||
| } | ||
|
|
||
| if has_plain { | ||
| Ok(PlaceholderMode::Plain { count: param_count }) | ||
| } else { | ||
| Ok(PlaceholderMode::Numbered { | ||
| max: numbered_max, | ||
| present: numbered_present, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Placeholder detection/validation/mapping are consistent and safe.
- Disallows mixing '?' and '?N'.
- Rejects named params (as intended).
- Builds a stable param_map for binding.
Add a brief doc comment on detect_placeholder_mode clarifying named params are intentionally unsupported per API.
Also applies to: 242-273, 275-312
🤖 Prompt for AI Agents
In packages/sqlite-web-core/src/database.rs around lines 171 to 240 (also apply
same change to 242-273 and 275-312), add a brief doc comment above
detect_placeholder_mode explaining that the function detects placeholder modes,
rejects named parameters intentionally (named params are not supported by this
API), disallows mixing plain '?' and numbered '?N' placeholders, and returns
either Plain { count } or Numbered { max, present }; update the comment to state
this behavior and the rationale that named params are intentionally unsupported
so callers understand the validation is deliberate.
There was a problem hiding this comment.
@findolor, understood! I'll skip the doc comment addition.
🧠 Learnings used
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.
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