Update library to take database name during construction #12
Conversation
WalkthroughImplements per-database naming: threads a sanitized DB name through worker generation, worker state, leadership/lock IDs, channel names, and OPFS initialization; updates public APIs, templates, tests, and adds identifier/file sanitizers and serde_json dependency. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App (Svelte/JS)
participant API as SQLiteWasmDatabase
participant WT as Worker Template
participant W as Web Worker
participant WS as WorkerState
participant DB as SQLiteDatabase
App->>API: new(db_name)
API->>WT: generate_self_contained_worker(db_name)
WT-->>W: worker code (self.__SQLITE_DB_NAME set)
W->>WS: init
WS->>WS: get_db_name_from_global -> sanitize_identifier
WS->>DB: initialize_opfs(db_name)
DB-->>WS: open/return
WS->>WS: create per-DB BroadcastChannel & lock-id
App->>API: execute query
API->>W: postMessage(query, lock-id for db_name)
W->>WS: route via per-DB channel
WS->>DB: run query
DB-->>WS: result
WS-->>API: result
API-->>App: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (4)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-09-15T06:11:31.713ZApplied to files:
🧬 Code graph analysis (2)packages/sqlite-web-core/src/database.rs (2)
packages/sqlite-web-core/src/coordination.rs (2)
⏰ 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)
🔇 Additional comments (12)
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 (7)
svelte-test/src/routes/sql/+page.svelte (1)
29-31: Avoid fixed sleeps; hook into worker-ready.The 1s sleep is brittle. Expose a ready signal from the library (e.g., db.waitUntilReady()) or listen for the "worker-ready" postMessage and await it here.
svelte-test/src/routes/+page.svelte (2)
29-32: Replace sleep with an explicit readiness await.Same as in sql console page—avoid fixed delay.
68-74: Do not interpolate user input into SQL — use parameterized queries.Replace the interpolated queries in svelte-test/src/routes/+page.svelte (lines 68–74; also 92–93) with parameter binding (preferred), or if the DB API lacks params, escape single quotes in string inputs and strictly coerce numeric ids before interpolation.
svelte-test/tests/fixtures/test-helpers.ts (1)
18-20: Avoid fixed sleep; prefer readiness await.If the library adds a ready API, use it here to speed up tests and reduce flakes.
packages/sqlite-web/src/lib.rs (2)
115-118: Out-of-order response bug: LIFO queue loses ordering.Using Vec::pop pairs the latest request with the earliest response. Switch to FIFO or add correlation IDs. Minimal fix below.
-use std::cell::RefCell; +use std::cell::RefCell; +use std::collections::VecDeque; @@ - pending_queries: Rc<RefCell<Vec<(js_sys::Function, js_sys::Function)>>>, + pending_queries: Rc<RefCell<VecDeque<(js_sys::Function, js_sys::Function)>>>, @@ - let pending_queries: Rc<RefCell<Vec<(js_sys::Function, js_sys::Function)>>> = - Rc::new(RefCell::new(Vec::new())); + let pending_queries: Rc<RefCell<VecDeque<(js_sys::Function, js_sys::Function)>>> = + Rc::new(RefCell::new(VecDeque::new())); @@ - pending_queries.borrow_mut().push((resolve, reject)); + pending_queries.borrow_mut().push_back((resolve, reject)); @@ - if let Some((resolve, reject)) = pending_queries_clone.borrow_mut().pop() { + if let Some((resolve, reject)) = pending_queries_clone.borrow_mut().pop_front() {If the worker can execute queries concurrently, prefer adding an incremental requestId and a HashMap<id, callbacks>.
Also applies to: 141-167
123-134: 'worker-error' is swallowed; reject pending promises.Currently returns early and leaves pending promises unresolved.
- } else if msg_type == "worker-error" { - if let Ok(_error) = js_sys::Reflect::get(&data, &JsValue::from_str("error")) - { - } - return; + } else if msg_type == "worker-error" { + let err_val = js_sys::Reflect::get(&data, &JsValue::from_str("error")).ok(); + let err_str = err_val + .and_then(|v| v.as_string()) + .unwrap_or_else(|| "Unknown worker error".to_string()); + // Drain and reject all pending + let mut q = pending_queries_clone.borrow_mut(); + while let Some((_resolve, reject)) = q.pop_front() { + let _ = reject.call1(&JsValue::NULL, &JsValue::from_str(&err_str)); + } + return; }packages/sqlite-web-core/src/coordination.rs (1)
149-171: Lock is held forever even if DB init fails (deadlock risk).On
initialize_opfserror you keepis_leader = trueand return a never‑resolving Promise, permanently holding the lock and blocking recovery.Release the lock on init failure and reset leadership; keep it only on success:
- let handler = Closure::once(move |_lock: JsValue| -> Promise { - *is_leader.borrow_mut() = true; - let db = Rc::clone(&db); - let channel = channel.clone(); - let worker_id = worker_id.clone(); - let db_name = db_name_for_handler.clone(); - - spawn_local(async move { - match SQLiteDatabase::initialize_opfs(&db_name).await { - Ok(database) => { - *db.borrow_mut() = Some(Rc::new(database)); - let msg = ChannelMessage::NewLeader { - leader_id: worker_id.clone(), - }; - let msg_js = serde_wasm_bindgen::to_value(&msg).unwrap(); - let _ = channel.post_message(&msg_js); - } - Err(_e) => {} - } - }); - - // Never resolve = hold lock forever - Promise::new(&mut |_, _| {}) - }); + let handler = Closure::once(move |_lock: JsValue| -> Promise { + *is_leader.borrow_mut() = true; + let db = Rc::clone(&db); + let channel = channel.clone(); + let worker_id = worker_id.clone(); + let db_name = db_name_for_handler.clone(); + let is_leader_clone = Rc::clone(&is_leader); + + // Hold lock only on success; resolve to release on failure. + Promise::new(&mut move |resolve, _reject| { + spawn_local(async move { + match SQLiteDatabase::initialize_opfs(&db_name).await { + Ok(database) => { + *db.borrow_mut() = Some(Rc::new(database)); + let msg = ChannelMessage::NewLeader { leader_id: worker_id.clone() }; + let msg_js = serde_wasm_bindgen::to_value(&msg).unwrap(); + let _ = channel.post_message(&msg_js); + // Intentionally do NOT resolve: keep the lock while leader is alive. + } + Err(_e) => { + *is_leader_clone.borrow_mut() = false; + let _ = resolve.call0(&JsValue::NULL); // release lock so others can retry + } + } + }); + }) + });Also applies to: 173-189, 208-216
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
packages/sqlite-web-core/src/coordination.rs(4 hunks)packages/sqlite-web-core/src/database.rs(3 hunks)packages/sqlite-web/Cargo.toml(1 hunks)packages/sqlite-web/src/lib.rs(7 hunks)packages/sqlite-web/src/worker_template.rs(1 hunks)svelte-test/src/routes/+page.svelte(1 hunks)svelte-test/src/routes/sql/+page.svelte(2 hunks)svelte-test/tests/fixtures/test-helpers.ts(2 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/fixtures/test-helpers.tssvelte-test/src/routes/sql/+page.sveltesvelte-test/src/routes/+page.svelte
🧠 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.rspackages/sqlite-web-core/src/coordination.rspackages/sqlite-web/src/lib.rs
🧬 Code graph analysis (3)
packages/sqlite-web-core/src/database.rs (2)
packages/sqlite-web-core/src/coordination.rs (1)
new(24-73)packages/sqlite-web/src/lib.rs (1)
new(87-179)
packages/sqlite-web-core/src/coordination.rs (2)
packages/sqlite-web/src/lib.rs (1)
new(87-179)packages/sqlite-web-core/src/database.rs (1)
initialize_opfs(15-72)
packages/sqlite-web/src/lib.rs (2)
packages/sqlite-web-core/src/coordination.rs (1)
new(24-73)packages/sqlite-web/src/worker_template.rs (1)
generate_self_contained_worker(4-11)
⏰ 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 (17)
svelte-test/src/routes/sql/+page.svelte (2)
22-22: DB name propagation to UI worker: looks good.Passing a concrete DB name aligns with the new API and multi-DB goal. Please confirm the chosen name matches any OPFS naming/sanitization rules used in core so paths/channels remain consistent.
509-509: No-op change.Trailing style marker change appears harmless.
packages/sqlite-web/Cargo.toml (1)
25-25: Duping serde_json in dev-deps is fine.Keeps tests consistent. No change requested.
svelte-test/src/routes/+page.svelte (1)
23-23: DB name passed to constructor: good alignment with API.Matches the per-DB naming objective.
svelte-test/tests/fixtures/test-helpers.ts (2)
6-6: Test helper accepts db name with sensible default: good.This enables per-test isolation and mirrors the API.
241-241: No-op change.Closing brace movement only.
packages/sqlite-web/src/lib.rs (6)
2-2: serde imports added for (de)serialization: OK.Matches new JSON-based init path.
35-49: Deserialization requires dbName: good validation; document JSON shape.The stricter shape is good. Please update API docs to show {"dbName": "..."}.
87-96: Constructor now enforces non-empty db_name: good guard.Trim + error is correct. No change requested.
95-95: Worker code includes DB name via template: OK. Ensure same sanitize rules across layers.Confirm this name matches the sanitization used in core/OPFS so identifiers/channels align.
262-262: Tests updated to pass db name: good coverage.These checks reflect the new API and worker template path.
Also applies to: 272-272, 280-280, 293-293, 322-322
183-212: Verify TypeScript return shape matches UI usage.UI reads result.value; the function is annotated
#[wasm_export(js_name = "query", unchecked_return_type = "string")]in packages/sqlite-web/src/lib.rs — confirm the generated .d.ts (build output, e.g., pkg/*.d.ts) exports the object shape{ value?: string; error?: ... }(not a plain string) to avoid TypeScript breakage.packages/sqlite-web-core/src/database.rs (1)
231-244: Tests updated to pass db name look good.The test harness now supplies a concrete name ("testdb"), matching the new API. LGTM.
Also applies to: 246-248
packages/sqlite-web-core/src/coordination.rs (4)
61-64: Per‑DB channel scoping looks correct.Namespacing
BroadcastChannelwith the sanitized DB name isolates traffic across databases.
70-71: Persisting raw db name in state is appropriate.Storing
db_nameenables later scoping (locks, init). LGTM.
194-207: Per‑DB lock identifier is well‑scoped.Lock name derivation matches channel scoping and avoids collisions across DBs. LGTM.
Also applies to: 210-213
25-43: Resolved — worker template injects __SQLITE_DB_NAME before WorkerState::new()generate_self_contained_worker prefixes
self.__SQLITE_DB_NAME = ...;before the embedded worker code (packages/sqlite-web/src/worker_template.rs:7); WorkerState::new() is called from the worker entrypoint (packages/sqlite-web-core/src/worker.rs:19).
| fn sanitize_for_id(name: &str) -> String { | ||
| let mut s: String = name | ||
| .trim() | ||
| .chars() | ||
| .map(|c| match c { | ||
| 'a'..='z' | 'A'..='Z' | '0'..='9' | '_' | '-' | '.' => c, | ||
| _ => '_', | ||
| }) | ||
| .collect(); | ||
| if s.is_empty() { | ||
| s.push_str("db"); | ||
| } | ||
| s | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Unify identifier/file-name sanitization.
sanitize_for_id is nearly the same as sanitize_filename in database.rs. Extract and reuse a single helper to avoid drift.
🤖 Prompt for AI Agents
In packages/sqlite-web-core/src/coordination.rs around lines 45 to 58,
sanitize_for_id duplicates the filename sanitization logic found in database.rs;
extract a single reusable helper (e.g., sanitize_identifier or
sanitize_filename) into a shared module (common/util or a new src/utils.rs) and
replace sanitize_for_id with a call to that helper; ensure the helper preserves
current behavior (trim, replace disallowed chars with '_', default to "db" when
empty) and update database.rs to use the new helper as well, exporting it with
pub(crate) so both modules compile.
| pub fn generate_self_contained_worker(db_name: &str) -> String { | ||
| // Safely JSON-encode the db name for JS embedding | ||
| let encoded = serde_json::to_string(db_name).unwrap_or_else(|_| "\"unknown\"".to_string()); | ||
| let prefix = format!("self.__SQLITE_DB_NAME = {};\n", encoded); | ||
| // Use the bundled worker template with embedded WASM | ||
| include_str!("embedded_worker.js").to_string() | ||
| let body = include_str!("embedded_worker.js"); | ||
| format!("{}{}", prefix, body) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Safe worker prefix injection: LGTM; harden against mutation.
Consider defining an immutable property to prevent accidental overwrite.
- let prefix = format!("self.__SQLITE_DB_NAME = {};\n", encoded);
+ let prefix = format!(
+ "Object.defineProperty(self, \"__SQLITE_DB_NAME\", {{ value: {}, writable: false, configurable: false }});\n",
+ encoded
+ );🤖 Prompt for AI Agents
In packages/sqlite-web/src/worker_template.rs around lines 4 to 11, instead of
injecting a mutable global assignment for self.__SQLITE_DB_NAME, define it as an
immutable, non-configurable property to harden against accidental overwrite;
change the prefix string to use Object.defineProperty(self, "__SQLITE_DB_NAME",
{ value: <encoded>, writable: false, configurable: false, enumerable: true });
so the encoded JSON db_name is used as the value and the property cannot be
reassigned or reconfigured.
There was a problem hiding this comment.
@findolor Understood! The current mutable assignment approach works fine for your use case. Thanks for the feedback.
|
|
||
| // Create database instance | ||
| const result = SQLiteWasmDatabase.new(); | ||
| const result = SQLiteWasmDatabase.new(name); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Constructor call with name: good. Add uniqueness for parallel tests.
To avoid cross-test collisions in OPFS/channels, consider a timestamp suffix by default.
-export async function createTestDatabase(name: string = 'ui-test-db'): Promise<SQLiteWasmDatabase> {
+export async function createTestDatabase(name: string = `ui-test-db-${Date.now()}`): Promise<SQLiteWasmDatabase> {📝 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.
| const result = SQLiteWasmDatabase.new(name); | |
| export async function createTestDatabase(name: string = `ui-test-db-${Date.now()}`): Promise<SQLiteWasmDatabase> { | |
| const result = SQLiteWasmDatabase.new(name); | |
| return result; | |
| } |
🤖 Prompt for AI Agents
In svelte-test/tests/fixtures/test-helpers.ts around line 11, the
SQLiteWasmDatabase.new(name) call uses a raw name which can collide across
parallel tests; modify the constructor invocation to append a unique suffix
(e.g., Date.now() or a short random hex) to the provided name so each test gets
a distinct DB/channel (e.g., `${name}-${Date.now()}` or
`${name}-${Math.random().toString(16).slice(2)}`) and use that composed string
when creating the database.
There was a problem hiding this comment.
no need to do this
There was a problem hiding this comment.
@findolor Understood! I'll note that test name uniqueness isn't a concern for this codebase.
✏️ Learnings added
Learnt from: findolor
PR: rainlanguage/sqlite-web#12
File: svelte-test/tests/fixtures/test-helpers.ts:11-11
Timestamp: 2025-09-15T10:01:17.694Z
Learning: In the sqlite-web project, test database names in createTestDatabase function do not need timestamp suffixes for uniqueness. The maintainer has confirmed that test isolation via unique naming is not required.
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.
Motivation
See issue: #8
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
fix #8
Summary by CodeRabbit
New Features
Tests
Chores