Fix library to include multiple SQL command support #9
Conversation
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/sqlite-web-core/src/coordination.rs (1)
148-151: Surface initialization errors for observability.Swallowing the error hides failures. At least log to console for debugging.
- Err(_e) => {} + Err(e) => web_sys::console::error_1(&JsValue::from_str(&format!( + "OPFS init failed: {e:?}" + ))),packages/sqlite-web-core/src/database.rs (3)
22-22: Remove unsafe Sync; it's unnecessary and potentially unsound.SQLiteDatabase wraps a raw, mutable sqlite3*; marking Sync claims it can be shared across threads safely, which isn’t guaranteed. The code runs on wasm and is single-threaded; drop Sync.
-unsafe impl Sync for SQLiteDatabase {}
79-215: Preserve column order in JSON objects.Building rows via HashMap loses column order. Use serde_json::Map to retain insertion order.
- let mut row_obj = std::collections::HashMap::new(); - let col_count = unsafe { sqlite3_column_count(stmt) }; + let mut row_obj = serde_json::Map::new(); + let col_count = unsafe { sqlite3_column_count(stmt) }; @@ - row_obj.insert(col_name.clone(), value); + row_obj.insert(col_name.clone(), value); @@ - results.push(serde_json::Value::Object(row_obj.into_iter().collect())); + results.push(serde_json::Value::Object(row_obj));
148-154: Float NaN/Infinity handling: avoid silent coercion to 0.Mapping invalid f64 to 0 can mislead. Consider serializing as null or string "NaN"/"Infinity".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/sqlite-web-core/src/coordination.rs(4 hunks)packages/sqlite-web-core/src/database.rs(20 hunks)
⏰ 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/coordination.rs (1)
17-17: Good move: own the DB and avoid RefCell borrows across await.Switching to Rc<RefCell<Option>> removes the inner Rc and enables &mut exec. This aligns with the new database API.
| spawn_local(async move { | ||
| let database = db.borrow().clone(); | ||
| let result = if let Some(database) = database { | ||
| database.exec(&sql).await | ||
| } else { | ||
| Err("Database not initialized".to_string()) | ||
| let result = { | ||
| // Extract database temporarily to avoid holding RefCell across await | ||
| let db_opt = db.borrow_mut().take(); | ||
| match db_opt { | ||
| Some(mut database) => { | ||
| let result = database.exec(&sql).await; | ||
| // Put the database back | ||
| *db.borrow_mut() = Some(database); | ||
| result | ||
| } | ||
| None => Err("Database not initialized".to_string()), | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure DB reinsertion even on early returns/panics (RAII guard).
Wrap the taken DB in a small guard that puts it back in Drop to avoid leaving it None if future aborts.
struct DbGuard<'a> {
cell: &'a Rc<RefCell<Option<SQLiteDatabase>>>,
db: Option<SQLiteDatabase>,
}
impl<'a> DbGuard<'a> {
fn take(cell: &'a Rc<RefCell<Option<SQLiteDatabase>>>) -> Result<Self, String> {
let db = cell.borrow_mut().take().ok_or("Database not initialized".to_string())?;
Ok(Self { cell, db: Some(db) })
}
fn db_mut(&mut self) -> &mut SQLiteDatabase { self.db.as_mut().unwrap() }
}
impl<'a> Drop for DbGuard<'a> {
fn drop(&mut self) {
if let Some(db) = self.db.take() {
*self.cell.borrow_mut() = Some(db);
}
}
}Use inside the critical sections instead of manual take/put.
Also applies to: 170-182
🤖 Prompt for AI Agents
In packages/sqlite-web-core/src/coordination.rs around lines 52-65 (and
similarly 170-182), the code takes the SQLiteDatabase out of the
Rc<RefCell<Option<...>>> and manually reinserts it after awaiting, which can
leave the cell None if the future aborts or panics; implement a small RAII
DbGuard type that takes the Option out on construction, exposes a mutable
reference to the inner SQLiteDatabase, and always reinserts the database into
the cell in its Drop implementation, then replace the manual take/put logic with
creating a DbGuard, calling guard.db_mut().exec(&sql).await (or other
operations) so the DB is restored even on early returns or panics.
Race: concurrent leader queries can see "Database not initialized".
While one task takes() the DB, a second concurrent task will read None and fail spuriously. Serialize access with an async mutex to avoid false errors.
Apply this diff (uses async-lock; add dependency in Cargo.toml: async-lock = "3"):
@@
-use crate::database::SQLiteDatabase;
+use crate::database::SQLiteDatabase;
+use async_lock::Mutex;
@@
pub struct WorkerState {
@@
- pub db: Rc<RefCell<Option<SQLiteDatabase>>>,
+ pub db: Rc<RefCell<Option<SQLiteDatabase>>>,
+ pub exec_lock: Rc<Mutex<()>>,
@@
pub fn new() -> Result<Self, JsValue> {
@@
Ok(WorkerState {
@@
db: Rc::new(RefCell::new(None)),
+ exec_lock: Rc::new(Mutex::new(())),
@@
pub fn setup_channel_listener(&self) {
@@
- let pending_queries = Rc::clone(&self.pending_queries);
+ let pending_queries = Rc::clone(&self.pending_queries);
+ let exec_lock = Rc::clone(&self.exec_lock);
@@
spawn_local(async move {
- let result = {
+ let _guard = exec_lock.lock().await;
+ let result = {
// Extract database temporarily to avoid holding RefCell across await
let db_opt = db.borrow_mut().take();
match db_opt {
Some(mut database) => {
let result = database.exec(&sql).await;
// Put the database back
*db.borrow_mut() = Some(database);
result
}
None => Err("Database not initialized".to_string()),
}
};
@@
pub async fn execute_query(&self, sql: String) -> Result<String, String> {
if *self.is_leader.borrow() {
- // Extract database temporarily to avoid holding RefCell across await
- let db_opt = self.db.borrow_mut().take();
+ let _guard = self.exec_lock.lock().await;
+ // Extract database temporarily to avoid holding RefCell across await
+ let db_opt = self.db.borrow_mut().take();
match db_opt {
Some(mut database) => {
let result = database.exec(&sql).await;
// Put the database back
- *self.db.borrow_mut() = Some(database);
+ *self.db.borrow_mut() = Some(database);
result
}
None => Err("Database not initialized".to_string()),
}Also applies to: 170-182
🤖 Prompt for AI Agents
In packages/sqlite-web-core/src/coordination.rs around lines 52-65 (and
similarly 170-182), concurrent tasks call borrow_mut().take() on the
RefCell-wrapped database which causes spurious "Database not initialized" errors
when another task has temporarily taken it; replace the RefCell approach with an
async-aware mutex: add async-lock = "3" to Cargo.toml, change the shared DB
container to an Arc<async_lock::Mutex<Option<YourDatabaseType>>> (or equivalent
async mutex), and in the spawned async tasks await a lock,
check/take/use/put-back the Option while holding the mutex (releasing the lock
after the awaited DB operation completes or, better, clone a handle if
available), ensuring all accesses are serialized to prevent the race.
There was a problem hiding this comment.
we don't have this error. we can safely use the db connection.
There was a problem hiding this comment.
@findolor, understood! Thanks for clarifying that this race condition doesn't occur in your implementation. I appreciate you sharing your experience with the database access pattern.
✏️ Learnings added
Learnt from: findolor
PR: rainlanguage/sqlite-web#9
File: packages/sqlite-web-core/src/coordination.rs:52-65
Timestamp: 2025-09-15T06:11:31.713Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| /// Execute potentially multiple SQL statements | ||
| pub async fn exec(&mut self, sql: &str) -> Result<String, String> { | ||
| if !sql.trim().ends_with(';') { | ||
| let (results, affected) = self.exec_single_statement(sql).await?; | ||
| return if let Some(results) = results { | ||
| serde_json::to_string_pretty(&results) | ||
| .map_err(|e| format!("JSON serialization error: {e}")) | ||
| } else { | ||
| Ok(format!( | ||
| "Query executed successfully. Rows affected: {affected}" | ||
| )) | ||
| }; | ||
| } | ||
|
|
||
| let statements = split_sql_statements(sql); | ||
|
|
||
| if statements.is_empty() { | ||
| return Ok("No statements to execute.".to_string()); | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Gate on trailing semicolon is surprising; document or drop.
Requiring a trailing semicolon to enable multi-statement execution is non-obvious and can cause partial execution (only the first statement) without warning. Either:
- Always split using the robust splitter; or
- Keep the gate but document prominently and emit a warning when extra statements are detected but ignored.
If you keep the gate, add a detection path:
- if !sql.trim().ends_with(';') {
+ let trimmed = sql.trim();
+ if !trimmed.ends_with(';') {
let (results, affected) = self.exec_single_statement(sql).await?;
+ // Warn if there are additional statements that will be ignored
+ if split_sql_statements(trimmed).len() > 1 {
+ web_sys::console::warn_1(&JsValue::from_str(
+ "Multiple SQL statements detected without trailing semicolon; only the first will run.",
+ ));
+ }📝 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.
| /// Execute potentially multiple SQL statements | |
| pub async fn exec(&mut self, sql: &str) -> Result<String, String> { | |
| if !sql.trim().ends_with(';') { | |
| let (results, affected) = self.exec_single_statement(sql).await?; | |
| return if let Some(results) = results { | |
| serde_json::to_string_pretty(&results) | |
| .map_err(|e| format!("JSON serialization error: {e}")) | |
| } else { | |
| Ok(format!( | |
| "Query executed successfully. Rows affected: {affected}" | |
| )) | |
| }; | |
| } | |
| let statements = split_sql_statements(sql); | |
| if statements.is_empty() { | |
| return Ok("No statements to execute.".to_string()); | |
| } | |
| /// Execute potentially multiple SQL statements | |
| pub async fn exec(&mut self, sql: &str) -> Result<String, String> { | |
| let trimmed = sql.trim(); | |
| if !trimmed.ends_with(';') { | |
| let (results, affected) = self.exec_single_statement(sql).await?; | |
| // Warn if there are additional statements that will be ignored | |
| if split_sql_statements(trimmed).len() > 1 { | |
| web_sys::console::warn_1(&JsValue::from_str( | |
| "Multiple SQL statements detected without trailing semicolon; only the first will run.", | |
| )); | |
| } | |
| return if let Some(results) = results { | |
| serde_json::to_string_pretty(&results) | |
| .map_err(|e| format!("JSON serialization error: {e}")) | |
| } else { | |
| Ok(format!( | |
| "Query executed successfully. Rows affected: {affected}" | |
| )) | |
| }; | |
| } | |
| let statements = split_sql_statements(sql); | |
| if statements.is_empty() { | |
| return Ok("No statements to execute.".to_string()); | |
| } |
There was a problem hiding this comment.
@findolor Thanks for confirming you'll address this! The warning approach or always splitting statements will definitely improve the user experience by avoiding the silent partial execution behavior.
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 (3)
packages/sqlite-web-core/src/database.rs (3)
13-13: Remove unsafe Sync; SQLite connection handles are not Sync.Marking SQLiteDatabase as Sync risks UB if used across threads. Methods already require &mut self; Sync is unnecessary and unsafe.
Apply this diff:
-unsafe impl Sync for SQLiteDatabase {}
100-106: Represent non‑finite floats as null instead of 0.from_f64(None) maps NaN/Inf; returning 0 is misleading.
Apply this diff:
- SQLITE_FLOAT => { - let val = unsafe { sqlite3_column_double(stmt, i) }; - serde_json::Value::Number( - serde_json::Number::from_f64(val) - .unwrap_or(serde_json::Number::from(0)), - ) - } + SQLITE_FLOAT => { + let val = unsafe { sqlite3_column_double(stmt, i) }; + if let Some(num) = serde_json::Number::from_f64(val) { + serde_json::Value::Number(num) + } else { + serde_json::Value::Null + } + }
92-132: Avoid double allocation when building JSON rows.HashMap → collect → serde_json::Map allocates twice.
Apply this diff:
- let mut row_obj = std::collections::HashMap::new(); + let mut row = serde_json::Map::with_capacity(col_count as usize); for i in 0..col_count { let col_type = unsafe { sqlite3_column_type(stmt, i) }; let value = match col_type { /* unchanged */ }; - if let Some(col_name) = column_names.get(i as usize) { - row_obj.insert(col_name.clone(), value); - } + if let Some(col_name) = column_names.get(i as usize) { + row.insert(col_name.clone(), value); + } } - - results.push(serde_json::Value::Object(row_obj.into_iter().collect())); + results.push(serde_json::Value::Object(row));
♻️ Duplicate comments (1)
packages/sqlite-web-core/src/database.rs (1)
206-221: Trailing‑semicolon gate is implemented; consider optional warning when extra statements are ignored.When no trailing semicolon, tails are silently ignored. Emitting a console warning would aid UX.
Option (WASM only):
if !trimmed.ends_with(';') { let (results, affected) = self.exec_single_statement(trimmed).await?; // If there were additional statements, warn. #[cfg(target_arch = "wasm32")] { if CString::new(sql).ok() .and_then(|c| { let mut stmt: *mut sqlite3_stmt = std::ptr::null_mut(); let mut tail: *const i8 = std::ptr::null(); let ret = unsafe { sqlite3_prepare_v2(self.db, c.as_ptr(), -1, &mut stmt, &mut tail) }; if ret == SQLITE_OK && !stmt.is_null() && !tail.is_null() && tail != c.as_ptr() { unsafe { sqlite3_finalize(stmt) }; Some(()) } else { None } }).is_some() { web_sys::console::warn_1(&JsValue::from_str( "Multiple SQL statements detected without trailing semicolon; only the first was executed.", )); } } /* existing return */ }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
packages/sqlite-web-core/src/database.rs(20 hunks)svelte-test/tests/integration/multi-sql-commands.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
svelte-test/**/*.{js,ts,svelte}
📄 CodeRabbit inference engine (CLAUDE.md)
Use an async/Promise-based API when interacting with the SQLite worker (await/Promises for query results)
Files:
svelte-test/tests/integration/multi-sql-commands.test.ts
🧠 Learnings (1)
📚 Learning: 2025-09-15T06:11:31.713Z
Learnt from: findolor
PR: rainlanguage/sqlite-web#9
File: packages/sqlite-web-core/src/coordination.rs:52-65
Timestamp: 2025-09-15T06:11:31.713Z
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.
Applied to files:
packages/sqlite-web-core/src/database.rs
🧬 Code graph analysis (2)
svelte-test/tests/integration/multi-sql-commands.test.ts (1)
svelte-test/tests/fixtures/test-helpers.ts (3)
createTestDatabase(6-22)cleanupDatabase(173-199)assertions(204-241)
packages/sqlite-web-core/src/database.rs (1)
packages/sqlite-web/src/lib.rs (1)
new(75-161)
⏰ 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 (3)
svelte-test/tests/integration/multi-sql-commands.test.ts (2)
12-18: Good async usage and lifecycle handling.beforeEach/afterEach correctly await the worker-backed API per our Svelte test guideline.
60-70: Gate behavior assertion is good; keep assertion tolerant to message wording.toContain("Rows affected: 1") + row count check is the right balance if the success prefix ever changes.
If you later change the success string, keep “Rows affected:” stable to avoid brittle tests.
packages/sqlite-web-core/src/database.rs (1)
294-299: Transaction state tracking via sqlite3_get_autocommit is correct.Updating in_transaction from the real autocommit mode avoids parser heuristics. Good.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
svelte-test/tests/fixtures/test-helpers.ts (1)
188-193: Make cleanup dynamic and quote identifiers to avoid maintenance and SQL injection on names.Static lists are brittle; also unquoted identifiers can break on special names. Prefer enumerating sqlite_master and dropping each object with proper quoting.
Apply:
- // Worker communication test tables - 'workers_test', 'shared_data', 'worker_coordination', 'message_test', - // Multi-SQL commands (UI) test tables - 'multi_ui', 'semi_ui', 'gate_ui', 'trg_src_ui', 'trg_log_ui', - // Database function test tables - 'bigint_test', 'categories', 'float_test', 'float_categories' - ]; - for (const table of tables) { - try { - await db.query(`DROP TABLE IF EXISTS ${table}`); - } catch (error) { - // Table might not exist, continue - } - } + // Dynamically drop all non-system tables and views + const res = await db.query( + "SELECT name, type FROM sqlite_master WHERE type IN ('table','view') AND name NOT LIKE 'sqlite_%'" + ); + const entries = (JSON.parse(res.value || '[]') as Array<{name: string; type: string}>); + for (const { name, type } of entries) { + const ddl = type === 'view' ? 'DROP VIEW' : 'DROP TABLE'; + try { + await db.query(`${ddl} IF EXISTS "${name.replace(/"/g, '""')}"`); + } catch { + // ignore and continue + } + }
📜 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 (4)
pkg/package.json(1 hunks)svelte-test/package.json(1 hunks)svelte-test/tests/fixtures/test-helpers.ts(1 hunks)svelte-test/tests/integration/multi-sql-commands.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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 (2)
📚 Learning: 2025-08-27T05:59:04.254Z
Learnt from: findolor
PR: rainlanguage/sqlite-web#5
File: svelte-test/tests/database-functions/bigint-sum.test.ts:2-7
Timestamp: 2025-08-27T05:59:04.254Z
Learning: In the sqlite-web repository, the user prefers to keep .js extensions when importing TypeScript files in test files, even in Vitest environments. This is acceptable and should not be flagged as an issue.
Applied to files:
svelte-test/tests/integration/multi-sql-commands.test.ts
📚 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 (1)
svelte-test/tests/integration/multi-sql-commands.test.ts (1)
svelte-test/tests/fixtures/test-helpers.ts (3)
createTestDatabase(6-22)cleanupDatabase(173-203)assertions(208-245)
⏰ 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 (4)
pkg/package.json (1)
15-15: No functional change; EOF newline only. LGTM.svelte-test/tests/integration/multi-sql-commands.test.ts (3)
20-35: Solid multi-statement happy-path assertion.Covers CREATE/INSERT/SELECT in a single call and validates only the final SELECT result is surfaced. Good.
37-72: Great coverage for semicolons inside strings, block comments, and line comments.This closes a common splitter bug class. 👍
86-108: Trigger case is well specified and order-sensitive. LGTM.
| "@rainlanguage/sqlite-web": "file:../pkg/rainlanguage-sqlite-web-0.0.1-alpha.0.tgz" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Tarball dependency aligns with our guideline for the Svelte test app.
This uses the locally packed tarball produced by npm pack, matching the repo’s instruction. I used the saved learning about this convention while reviewing.
Run to verify the tarball exists, matches name/version, and contains the wasm/js/d.ts:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
TARBALL_PATH=$(node -p "require('./svelte-test/package.json').dependencies['@rainlanguage/sqlite-web'].replace(/^file:/,'')")
echo "Checking $TARBALL_PATH"
test -f "$TARBALL_PATH" || { echo "Missing tarball at $TARBALL_PATH"; exit 1; }
# Inspect package.json inside tarball
tar -xOf "$TARBALL_PATH" package/package.json | tee /tmp/sqlite-web-pkg.json >/dev/null
echo "Packed name/version:"; jq -r '.name + "@" + .version' /tmp/sqlite-web-pkg.json
# Ensure key artifacts are included
echo "Verifying wasm/js/typings presence:"
tar -tf "$TARBALL_PATH" | rg -n 'sqlite_web_bg\.wasm|sqlite_web\.js|sqlite_web\.d\.ts' || { echo "Expected artifacts missing"; exit 1; }Length of output: 125
Missing packed tarball referenced by svelte-test/package.json
Verification failed: ../pkg/rainlanguage-sqlite-web-0.0.1-alpha.0.tgz not found. Generate the tarball (run npm pack in pkg to produce rainlanguage-sqlite-web-0.0.1-alpha.0.tgz) or update svelte-test/package.json to point to a valid tarball/registry version; then re-run verification to confirm package/package.json and sqlite_web_bg.wasm, sqlite_web.js, sqlite_web.d.ts are present.
🤖 Prompt for AI Agents
In svelte-test/package.json around lines 44-45, the dependency points to a
non-existent local tarball ../pkg/rainlanguage-sqlite-web-0.0.1-alpha.0.tgz;
either run npm pack in the pkg directory to generate that tarball (ensuring
sqlite_web_bg.wasm, sqlite_web.js, sqlite_web.d.ts are produced and the filename
matches), or update svelte-test/package.json to reference a valid tarball path
or published registry version, then re-run verification.
| describe("Multi-SQL Commands (UI Integration)", () => { | ||
| let db: SQLiteWasmDatabase; | ||
|
|
||
| beforeEach(async () => { | ||
| db = await createTestDatabase(); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| if (db) await cleanupDatabase(db); | ||
| }); | ||
|
|
||
| it("executes multiple statements in a single query call", async () => { | ||
| const sql = ` | ||
| CREATE TABLE multi_ui (id INTEGER PRIMARY KEY, name TEXT); | ||
| INSERT INTO multi_ui (name) VALUES ('Alice'); | ||
| INSERT INTO multi_ui (name) VALUES ('Bob'); | ||
| SELECT COUNT(*) as count FROM multi_ui; | ||
| `; | ||
|
|
||
| const result = await db.query(sql); | ||
| expect(result.error).toBeUndefined(); | ||
|
|
||
| const data = JSON.parse(result.value || "[]"); | ||
| expect(Array.isArray(data)).toBe(true); | ||
| expect(data).toHaveLength(1); | ||
| expect(data[0].count).toBe(2); | ||
| }); | ||
|
|
||
| it("doesn't split semicolons inside strings or comments", async () => { | ||
| await db.query( | ||
| `CREATE TABLE semi_ui (id INTEGER PRIMARY KEY, name TEXT, note TEXT)`, | ||
| ); | ||
|
|
||
| const multi = ` | ||
| INSERT INTO semi_ui (name, note) VALUES ('A', 'hello; world'); | ||
| /* block; comment; with; semicolons */ | ||
| INSERT INTO semi_ui (name, note) VALUES ('B', '-- not a comment ; inside string'); | ||
| `; | ||
|
|
||
| const res = await db.query(multi); | ||
| expect(res.error).toBeUndefined(); | ||
|
|
||
| const rows = await db.query(`SELECT name, note FROM semi_ui ORDER BY id`); | ||
| const data = JSON.parse(rows.value || "[]"); | ||
| expect(data).toHaveLength(2); | ||
| expect(data[0].name).toBe("A"); | ||
| expect(data[0].note).toBe("hello; world"); | ||
| expect(data[1].name).toBe("B"); | ||
| expect(data[1].note).toBe("-- not a comment ; inside string"); | ||
|
|
||
| // Line comment with semicolons should not split statements | ||
| const lineCommentAndInsert = ` | ||
| -- pre;line;comment | ||
| INSERT INTO semi_ui (name, note) VALUES ('C', 'third row'); | ||
| `; | ||
| const res2 = await db.query(lineCommentAndInsert); | ||
| expect(res2.error).toBeUndefined(); | ||
|
|
||
| const countRes = await db.query(`SELECT COUNT(*) c FROM semi_ui`); | ||
| const countData = JSON.parse(countRes.value || "[]"); | ||
| expect(Array.isArray(countData)).toBe(true); | ||
| expect(countData).toHaveLength(1); | ||
| expect(countData[0].c).toBe(3); | ||
| }); | ||
|
|
||
| it("gates multi-statement execution by trailing semicolon", async () => { | ||
| await db.query(`CREATE TABLE gate_ui (id INTEGER)`); | ||
|
|
||
| const gated = `INSERT INTO gate_ui (id) VALUES (1); DELETE FROM gate_ui WHERE id = 1`; | ||
| const res = await db.query(gated); | ||
|
|
||
| expect(res.error).toBeUndefined(); | ||
| expect(res.value || "").toContain("Rows affected: 1"); | ||
|
|
||
| await assertions.assertRowCount(db, "SELECT * FROM gate_ui", 1); | ||
| }); | ||
|
|
||
| it("supports triggers with semicolons in the body (BEGIN ... END)", async () => { | ||
| await db.query(`CREATE TABLE trg_src_ui (id INTEGER)`); | ||
| await db.query(`CREATE TABLE trg_log_ui (msg TEXT)`); | ||
|
|
||
| const triggerSql = ` | ||
| CREATE TRIGGER trg_ui AFTER INSERT ON trg_src_ui | ||
| BEGIN | ||
| INSERT INTO trg_log_ui (msg) VALUES ('first; message'); | ||
| INSERT INTO trg_log_ui (msg) VALUES ('second; message'); | ||
| END; | ||
| `; | ||
|
|
||
| const createRes = await db.query(triggerSql); | ||
| expect(createRes.error).toBeUndefined(); | ||
|
|
||
| await db.query(`INSERT INTO trg_src_ui (id) VALUES (1)`); | ||
|
|
||
| const logRows = await db.query(`SELECT msg FROM trg_log_ui ORDER BY rowid`); | ||
| const logData = JSON.parse(logRows.value || "[]"); | ||
| expect(logData).toHaveLength(2); | ||
| expect(logData[0].msg).toBe("first; message"); | ||
| expect(logData[1].msg).toBe("second; message"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick
Add a rollback-on-error scenario to lock in transactional semantics.
Given core changes added transaction-aware multi-statement execution, add one failure-in-the-middle test to ensure no partial effects.
Example:
it("rolls back multi-statement batch when a middle statement fails", async () => {
await db.query(`CREATE TABLE fail_ui (id INTEGER PRIMARY KEY, name TEXT)`);
const script = `
INSERT INTO fail_ui (name) VALUES ('ok1');
INSERT INTO no_such_table (x) VALUES (1);
INSERT INTO fail_ui (name) VALUES ('ok2');
`;
const res = await db.query(script);
expect(res.error).toBeDefined();
const rows = JSON.parse((await db.query(`SELECT * FROM fail_ui`)).value || "[]");
expect(rows).toHaveLength(0); // nothing committed
});🤖 Prompt for AI Agents
In svelte-test/tests/integration/multi-sql-commands.test.ts lines 9-109, add a
test that verifies a multi-statement batch is rolled back when a middle
statement fails: create a table fail_ui, run a multi-statement script that
inserts a row, then references a non-existent table to force an error, then
attempts another insert; assert the query returned an error and then query
fail_ui to assert zero rows (ensuring no partial commits), mirroring the
provided example test logic and assertions.
| beforeEach(async () => { | ||
| db = await createTestDatabase(); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| if (db) await cleanupDatabase(db); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick
Potential flakiness from worker readiness in createTestDatabase(); consider a readiness probe.
beforeEach relies on a fixed 1s sleep in createTestDatabase. Replace with a trivial probe (e.g., SELECT 1) to deterministically await readiness.
Apply in tests/fixtures/test-helpers.ts:
- // Wait for worker to be ready
- await new Promise(resolve => setTimeout(resolve, 1000));
+ // Wait for worker to be ready deterministically
+ try { await db.query('SELECT 1'); } catch { /* retry once */ await db.query('SELECT 1'); }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In svelte-test/tests/integration/multi-sql-commands.test.ts around lines 12 to
18, the beforeEach uses createTestDatabase which currently relies on a fixed 1s
sleep and is flaky; update createTestDatabase in tests/fixtures/test-helpers.ts
to replace the fixed sleep with a deterministic readiness probe that repeatedly
runs a trivial query (e.g., SELECT 1) against the worker with a short interval
until it succeeds or a reasonable timeout is reached, returning the DB only
after the probe succeeds; ensure the probe catches transient errors and fails
the test with a clear timeout error if readiness is not achieved.
| it("gates multi-statement execution by trailing semicolon", async () => { | ||
| await db.query(`CREATE TABLE gate_ui (id INTEGER)`); | ||
|
|
||
| const gated = `INSERT INTO gate_ui (id) VALUES (1); DELETE FROM gate_ui WHERE id = 1`; | ||
| const res = await db.query(gated); | ||
|
|
||
| expect(res.error).toBeUndefined(); | ||
| expect(res.value || "").toContain("Rows affected: 1"); | ||
|
|
||
| await assertions.assertRowCount(db, "SELECT * FROM gate_ui", 1); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick
Useful gating behavior test; consider asserting non-execution explicitly.
You already assert one row remains. For extra clarity, also assert DELETE didn’t run by checking value contains only one “Rows affected: 1”.
- expect(res.value || "").toContain("Rows affected: 1");
+ const out = res.value || "";
+ expect(out).toContain("Rows affected: 1");
+ expect(out.match(/Rows affected: 1/g)?.length).toBe(1);📝 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.
| it("gates multi-statement execution by trailing semicolon", async () => { | |
| await db.query(`CREATE TABLE gate_ui (id INTEGER)`); | |
| const gated = `INSERT INTO gate_ui (id) VALUES (1); DELETE FROM gate_ui WHERE id = 1`; | |
| const res = await db.query(gated); | |
| expect(res.error).toBeUndefined(); | |
| expect(res.value || "").toContain("Rows affected: 1"); | |
| await assertions.assertRowCount(db, "SELECT * FROM gate_ui", 1); | |
| }); | |
| it("gates multi-statement execution by trailing semicolon", async () => { | |
| await db.query(`CREATE TABLE gate_ui (id INTEGER)`); | |
| const gated = `INSERT INTO gate_ui (id) VALUES (1); DELETE FROM gate_ui WHERE id = 1`; | |
| const res = await db.query(gated); | |
| expect(res.error).toBeUndefined(); | |
| const out = res.value || ""; | |
| expect(out).toContain("Rows affected: 1"); | |
| expect(out.match(/Rows affected: 1/g)?.length).toBe(1); | |
| await assertions.assertRowCount(db, "SELECT * FROM gate_ui", 1); | |
| }); |
🤖 Prompt for AI Agents
In svelte-test/tests/integration/multi-sql-commands.test.ts around lines 74–84,
add an explicit assertion that the DELETE did not execute by checking the query
result string contains only the single success message for the INSERT; e.g.,
after the existing expects assert that (res.value || "") does not contain
indicators of deletion and contains exactly one "Rows affected: 1" (use a string
non-occurrence check or count/match to ensure there is only one occurrence).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
svelte-test/tests/fixtures/test-helpers.ts (2)
27-41: Do not split SQL by semicolons in TS; let the backend handle multi-statements.This naive split breaks on semicolons inside strings/comments and diverges from the robust prepare/tail parsing you added in Rust. Execute the script as a single call and rely on the backend’s multi-statement executor.
-export async function executeSqlScript(db: SQLiteWasmDatabase, sqlContent: string): Promise<void> { - // Split SQL content by semicolons and execute each statement - const statements = sqlContent - .split(';') - .map(stmt => stmt.trim()) - .filter(stmt => stmt.length > 0); - - for (const statement of statements) { - try { - await db.query(statement); - } catch (error) { - console.warn(`Failed to execute statement: ${statement}`, error); - } - } -} +export async function executeSqlScript(db: SQLiteWasmDatabase, sqlContent: string): Promise<void> { + try { + await db.query(sqlContent); + } catch (error) { + console.warn('Failed to execute SQL script', error); + } +}
173-193: Batch drops in a single transaction to speed up cleanup.Wrapping the DROP loop in BEGIN/COMMIT reduces fs round-trips and ensures atomic cleanup.
export async function cleanupDatabase(db: SQLiteWasmDatabase): Promise<void> { if (!db) return; try { - // Drop all test tables + // Drop all test tables const tables = [ @@ - for (const table of tables) { - try { - await db.query(`DROP TABLE IF EXISTS ${table}`); - } catch (error) { - // Table might not exist, continue - } - } + await db.query('BEGIN'); + try { + for (const table of tables) { + await db.query(`DROP TABLE IF EXISTS ${table}`); + } + await db.query('COMMIT'); + } catch { + await db.query('ROLLBACK'); + }packages/sqlite-web-core/src/database.rs (1)
118-123: Prefer null over 0 for non-finite floats in JSON.Mapping NaN/Inf to 0 is misleading. Use null to preserve semantics.
- serde_json::Value::Number( - serde_json::Number::from_f64(val) - .unwrap_or(serde_json::Number::from(0)), - ) + match serde_json::Number::from_f64(val) { + Some(n) => serde_json::Value::Number(n), + None => serde_json::Value::Null, + }
♻️ Duplicate comments (1)
packages/sqlite-web-core/src/coordination.rs (1)
78-89: Use an RAII guard to ensure DB is always reinserted (even on early returns/panics).Same suggestion as before; avoids leaving db = None if control flow skips reinsertion.
+struct DbGuard<'a> { + cell: &'a Rc<RefCell<Option<SQLiteDatabase>>>, + db: Option<SQLiteDatabase>, +} +impl<'a> DbGuard<'a> { + fn take(cell: &'a Rc<RefCell<Option<SQLiteDatabase>>>) -> Result<Self, String> { + let db = cell.borrow_mut().take().ok_or("Database not initialized".to_string())?; + Ok(Self { cell, db: Some(db) }) + } + fn db_mut(&mut self) -> &mut SQLiteDatabase { self.db.as_mut().unwrap() } +} +impl<'a> Drop for DbGuard<'a> { + fn drop(&mut self) { + if let Some(db) = self.db.take() { + *self.cell.borrow_mut() = Some(db); + } + } +} @@ - spawn_local(async move { - let result = { - // Extract database temporarily to avoid holding RefCell across await - let db_opt = db.borrow_mut().take(); - match db_opt { - Some(mut database) => { - let result = database.exec(&sql).await; - // Put the database back - *db.borrow_mut() = Some(database); - result - } - None => Err("Database not initialized".to_string()), - } - }; + spawn_local(async move { + let result = match DbGuard::take(&db) { + Ok(mut guard) => guard.db_mut().exec(&sql).await, + Err(e) => Err(e), + }; @@ - // Extract database temporarily to avoid holding RefCell across await - let db_opt = self.db.borrow_mut().take(); - match db_opt { - Some(mut database) => { - let result = database.exec(&sql).await; - // Put the database back - *self.db.borrow_mut() = Some(database); - result - } - None => Err("Database not initialized".to_string()), - } + match DbGuard::take(&self.db) { + Ok(mut guard) => guard.db_mut().exec(&sql).await, + Err(e) => Err(e), + }Also applies to: 200-207
📜 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/coordination.rs(4 hunks)packages/sqlite-web-core/src/database.rs(20 hunks)svelte-test/tests/fixtures/test-helpers.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-15T06:11:31.781Z
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.
Applied to files:
packages/sqlite-web-core/src/coordination.rspackages/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/worker.rs : Keep worker.rs as the main worker entry point invoked by worker_main()
Applied to files:
packages/sqlite-web-core/src/coordination.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/coordination.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/coordination.rspackages/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 (1)
packages/sqlite-web-core/src/database.rs (1)
packages/sqlite-web-core/src/database_functions/mod.rs (1)
register_custom_functions(20-82)
⏰ 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 (2)
svelte-test/tests/fixtures/test-helpers.ts (1)
188-193: UI test tables added — LGTM.Naming is consistent and scoped to UI multi-SQL tests.
packages/sqlite-web-core/src/database.rs (1)
63-67: Good: close DB if custom-function registration fails.Prevents handle leaks on init errors.
| // Single-statement mode: execute only the first statement, ignore tail | ||
| if !trimmed.ends_with(';') { | ||
| let (results, affected) = self.exec_single_statement(trimmed).await?; | ||
|
|
||
| // Return results | ||
| if sql.trim().to_lowercase().starts_with("select") && !results.is_empty() { | ||
| // Update transaction state based on actual DB autocommit mode | ||
| self.in_transaction = unsafe { sqlite3_get_autocommit(self.db) } == 0; | ||
|
|
||
| return if let Some(results) = results { | ||
| serde_json::to_string_pretty(&results) | ||
| .map_err(|e| format!("JSON serialization error: {e}")) | ||
| } else { | ||
| Ok(format!( | ||
| "Query executed successfully. Rows affected: {affected}" | ||
| )) | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Warn when extra statements are present in “single-statement” mode.
Currently, tail statements are silently ignored if input lacks a trailing semicolon. Emit a console warning based on the tail pointer to avoid surprising partial execution.
async fn exec_single_statement(
&self,
sql: &str,
) -> Result<(Option<Vec<serde_json::Value>>, i32), String> {
let sql_cstr = CString::new(sql).map_err(|e| format!("Invalid SQL string: {e}"))?;
let mut ptr = sql_cstr.as_ptr();
loop {
let mut stmt: *mut sqlite3_stmt = std::ptr::null_mut();
let mut tail: *const i8 = std::ptr::null();
@@
- // Execute only the first prepared statement; ignore any tail
- return self.exec_prepared_statement(stmt);
+ // Execute only the first prepared statement; warn if tail contains more tokens
+ let res = self.exec_prepared_statement(stmt);
+ if !tail.is_null() && tail != ptr {
+ // Safe because tail points into sql_cstr's buffer
+ let leftover = unsafe { CStr::from_ptr(tail) }.to_string_lossy();
+ if leftover.trim().len() > 0 {
+ web_sys::console::warn_1(&JsValue::from_str(
+ "Multiple SQL statements detected without trailing semicolon; only the first was executed.",
+ ));
+ }
+ }
+ return res;
}
}
/// Execute potentially multiple SQL statements
pub async fn exec(&mut self, sql: &str) -> Result<String, String> {
let trimmed = sql.trim();
// Single-statement mode: execute only the first statement, ignore tail
if !trimmed.ends_with(';') {
let (results, affected) = self.exec_single_statement(trimmed).await?;Also applies to: 178-216
🤖 Prompt for AI Agents
In packages/sqlite-web-core/src/database.rs around lines 223-238 (also apply
same change to 178-216), the single-statement branch silently ignores any
trailing SQL after the first statement when the input lacks a trailing
semicolon; modify this path to detect the tail (the remaining unexecuted slice
starting at the tail pointer) and emit a console warning that includes either
the tail text (truncated to a safe length) or its start byte/char position so
callers are aware of partial execution; keep existing execution and
transaction-state behavior unchanged, ensure the warning is conditional (only
when tail is non-empty/contains non-whitespace), and use the existing
logging/console mechanism consistent with the crate’s conventions.
Motivation
See issue: #7
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores