Fix follower timeout logic #23
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughAdds leader-aware worker coordination and structured worker errors; implements a WASM-backed SQLite client with ReadySignal, parameter normalization, worker-template follower timeout injection, updated worker lifecycle messaging, and widespread tests and example updates to await readiness. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App (Svelte)
participant DB as SQLiteWasmDatabase
participant MainW as Main Thread Worker
participant CoreW as Worker Scope
participant Ready as ReadySignal
App->>DB: new(db_name) (async)
DB->>MainW: create_worker_from_code(...)
MainW->>CoreW: spawn worker, init WorkerState (Follower, Unknown)
CoreW->>CoreW: start_leader_probe() periodically (uses follower_timeout_ms)
alt leader discovered
CoreW->>CoreW: set is_leader / has_leader = Known
CoreW->>Ready: mark_ready()
CoreW->>MainW: postMessage { type: "worker-ready" }
else probe timeout / no leader
CoreW->>MainW: postMessage { type: "worker-error", error: InitializationPending }
CoreW->>Ready: mark_failed(...)
end
Ready-->>DB: Promise resolves/rejects
DB->>App: new() resolves/rejects
App->>DB: query(sql, params)
DB->>MainW: postMessage { type: "execute-query", requestId, ... }
MainW->>CoreW: deliver execute-query
CoreW-->>MainW: postMessage { type: "query-result", requestId, result/error }
MainW->>DB: resolve/reject pending promise
DB->>App: query result or mapped InitializationPending error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas to focus on:
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 (1)
🧰 Additional context used🧠 Learnings (11)📓 Common learnings📚 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:
📚 Learning: 2025-09-17T06:55:20.178ZApplied to files:
📚 Learning: 2025-09-15T06:11:31.781ZApplied to files:
📚 Learning: 2025-10-20T12:24:07.874ZApplied to files:
📚 Learning: 2025-09-17T06:55:20.178ZApplied to files:
📚 Learning: 2025-09-19T07:06:20.173ZApplied to files:
📚 Learning: 2025-08-27T05:55:40.481ZApplied to files:
🧬 Code graph analysis (1)packages/sqlite-web/src/db.rs (6)
⏰ 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). (1)
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: 6
📜 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 (18)
packages/sqlite-web-core/src/coordination.rs(14 hunks)packages/sqlite-web-core/src/messages.rs(2 hunks)packages/sqlite-web-core/src/worker.rs(2 hunks)packages/sqlite-web/src/db.rs(1 hunks)packages/sqlite-web/src/errors.rs(1 hunks)packages/sqlite-web/src/lib.rs(1 hunks)packages/sqlite-web/src/params.rs(1 hunks)packages/sqlite-web/src/ready.rs(1 hunks)packages/sqlite-web/src/tests.rs(1 hunks)packages/sqlite-web/src/utils.rs(1 hunks)packages/sqlite-web/src/worker.rs(1 hunks)packages/sqlite-web/src/worker_template.rs(1 hunks)pkg/package.json(1 hunks)scripts/local-bundle.sh(0 hunks)svelte-test/package.json(1 hunks)svelte-test/src/routes/+page.svelte(1 hunks)svelte-test/src/routes/sql/+page.svelte(1 hunks)svelte-test/tests/fixtures/test-helpers.ts(1 hunks)
💤 Files with no reviewable changes (1)
- scripts/local-bundle.sh
🧰 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
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
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 (17)
📓 Common learnings
Learnt from: CR
Repo: rainlanguage/sqlite-web PR: 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
Repo: rainlanguage/sqlite-web PR: 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
Repo: rainlanguage/sqlite-web PR: 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: findolor
Repo: rainlanguage/sqlite-web PR: 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
Repo: rainlanguage/sqlite-web PR: 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
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
Repo: rainlanguage/sqlite-web PR: 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/src/utils.rspackages/sqlite-web/src/errors.rspackages/sqlite-web/src/params.rspackages/sqlite-web/src/worker_template.rspackages/sqlite-web/src/tests.rspackages/sqlite-web/src/db.rspackages/sqlite-web/src/lib.rspackages/sqlite-web-core/src/coordination.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
Repo: rainlanguage/sqlite-web PR: 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/utils.rspackages/sqlite-web/src/errors.rspackages/sqlite-web/src/ready.rssvelte-test/src/routes/sql/+page.sveltepackages/sqlite-web-core/src/worker.rspackages/sqlite-web/src/params.rspackages/sqlite-web/src/worker_template.rssvelte-test/src/routes/+page.sveltepackages/sqlite-web/src/tests.rspackages/sqlite-web/src/db.rspackages/sqlite-web/src/worker.rspackages/sqlite-web/src/lib.rspackages/sqlite-web-core/src/coordination.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
Repo: rainlanguage/sqlite-web PR: 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/utils.rspackages/sqlite-web/src/errors.rspackages/sqlite-web/src/ready.rssvelte-test/src/routes/sql/+page.sveltesvelte-test/tests/fixtures/test-helpers.tspackages/sqlite-web/src/params.rspackages/sqlite-web/src/worker_template.rssvelte-test/src/routes/+page.sveltepackages/sqlite-web/src/tests.rspackages/sqlite-web/src/db.rspackages/sqlite-web/src/worker.rspackages/sqlite-web/src/lib.rspackages/sqlite-web-core/src/coordination.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
Repo: rainlanguage/sqlite-web PR: 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/utils.rspackages/sqlite-web-core/src/messages.rspackages/sqlite-web/src/errors.rspackages/sqlite-web/src/ready.rssvelte-test/src/routes/sql/+page.sveltepackages/sqlite-web-core/src/worker.rspackages/sqlite-web/src/params.rspackages/sqlite-web/src/worker_template.rssvelte-test/src/routes/+page.sveltepackages/sqlite-web/src/tests.rspackages/sqlite-web/src/db.rspackages/sqlite-web/src/worker.rspackages/sqlite-web/src/lib.rspackages/sqlite-web-core/src/coordination.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
Repo: rainlanguage/sqlite-web PR: 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.jsonsvelte-test/src/routes/sql/+page.sveltesvelte-test/tests/fixtures/test-helpers.tssvelte-test/src/routes/+page.sveltepackages/sqlite-web/src/tests.rs
📚 Learning: 2025-08-27T05:59:04.254Z
Learnt from: findolor
Repo: rainlanguage/sqlite-web PR: 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/package.jsonsvelte-test/src/routes/sql/+page.sveltesvelte-test/tests/fixtures/test-helpers.ts
📚 Learning: 2025-08-21T15:09:22.737Z
Learnt from: findolor
Repo: rainlanguage/sqlite-web PR: 2
File: flake.nix:59-68
Timestamp: 2025-08-21T15:09:22.737Z
Learning: In the sqlite-web project, the build-submodules task in flake.nix successfully builds the lib/rain.math.float submodule with `forge build` even if foundry.toml is not immediately visible in the expected location. The build process works as intended according to the project maintainer.
Applied to files:
svelte-test/package.json
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
Repo: rainlanguage/sqlite-web PR: 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/errors.rspackages/sqlite-web/src/ready.rssvelte-test/src/routes/sql/+page.sveltepackages/sqlite-web-core/src/worker.rspackages/sqlite-web/src/params.rspackages/sqlite-web/src/worker_template.rssvelte-test/src/routes/+page.sveltepackages/sqlite-web/src/tests.rspackages/sqlite-web/src/db.rspackages/sqlite-web/src/worker.rspackages/sqlite-web/src/lib.rspackages/sqlite-web-core/src/coordination.rs
📚 Learning: 2025-09-19T07:06:20.173Z
Learnt from: findolor
Repo: rainlanguage/sqlite-web PR: 14
File: svelte-test/tests/database-functions/float-is-zero.test.ts:39-57
Timestamp: 2025-09-19T07:06:20.173Z
Learning: PRAGMA function_list is not available in the WASM SQLite test environment used by svelte-test, so availability testing should rely on direct function calls rather than PRAGMA queries.
Applied to files:
packages/sqlite-web/src/errors.rssvelte-test/src/routes/sql/+page.sveltesvelte-test/tests/fixtures/test-helpers.tspackages/sqlite-web/src/params.rssvelte-test/src/routes/+page.sveltepackages/sqlite-web/src/tests.rspackages/sqlite-web/src/db.rspackages/sqlite-web/src/lib.rs
📚 Learning: 2025-09-15T06:11:31.781Z
Learnt from: findolor
Repo: rainlanguage/sqlite-web PR: 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/src/ready.rssvelte-test/src/routes/sql/+page.sveltepackages/sqlite-web/src/worker_template.rssvelte-test/src/routes/+page.sveltepackages/sqlite-web/src/tests.rspackages/sqlite-web/src/db.rspackages/sqlite-web/src/lib.rspackages/sqlite-web-core/src/coordination.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
Repo: rainlanguage/sqlite-web PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web/embedded_worker.js : Do not manually edit embedded_worker.js; it is generated by the build and should be treated as read-only
Applied to files:
svelte-test/src/routes/sql/+page.sveltepackages/sqlite-web/src/worker_template.rssvelte-test/src/routes/+page.sveltepackages/sqlite-web/src/db.rspackages/sqlite-web/src/worker.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
Repo: rainlanguage/sqlite-web PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Maintain self-contained workers with no external WASM file dependencies across the project
Applied to files:
svelte-test/src/routes/sql/+page.sveltepackages/sqlite-web/src/worker_template.rspackages/sqlite-web/src/db.rspackages/sqlite-web/src/worker.rs
📚 Learning: 2025-09-15T10:01:17.743Z
Learnt from: findolor
Repo: rainlanguage/sqlite-web PR: 12
File: svelte-test/tests/fixtures/test-helpers.ts:11-11
Timestamp: 2025-09-15T10:01:17.743Z
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.
Applied to files:
svelte-test/tests/fixtures/test-helpers.ts
📚 Learning: 2025-09-17T08:04:44.062Z
Learnt from: findolor
Repo: rainlanguage/sqlite-web PR: 6
File: svelte-test/tests/database-functions/float-sum.test.ts:78-86
Timestamp: 2025-09-17T08:04:44.062Z
Learning: In svelte-test/tests/database-functions/float-sum.test.ts, the schema defines `amount TEXT NOT NULL` but the NULL value handling tests attempt to insert NULL values, creating a constraint violation that prevents the tests from passing.
Applied to files:
svelte-test/tests/fixtures/test-helpers.tspackages/sqlite-web/src/lib.rs
📚 Learning: 2025-10-20T12:24:07.874Z
Learnt from: findolor
Repo: rainlanguage/sqlite-web PR: 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/src/params.rspackages/sqlite-web/src/db.rspackages/sqlite-web/src/lib.rs
📚 Learning: 2025-09-17T17:44:54.855Z
Learnt from: findolor
Repo: rainlanguage/sqlite-web PR: 15
File: svelte-test/tests/integration/error-handling.test.ts:418-434
Timestamp: 2025-09-17T17:44:54.855Z
Learning: In the sqlite-web test harness, parameter binding is not available for the db.query() method, so string interpolation with manual SQL escaping using value.replace(/'/g, "''") is the necessary approach for handling special characters in test data.
Applied to files:
packages/sqlite-web/src/db.rspackages/sqlite-web/src/lib.rs
🧬 Code graph analysis (8)
packages/sqlite-web/src/errors.rs (3)
packages/sqlite-web/src/worker.rs (1)
serde_wasm_bindgen(58-58)packages/sqlite-web/src/db.rs (1)
new(42-52)packages/sqlite-web/src/ready.rs (1)
new(25-40)
packages/sqlite-web/src/ready.rs (2)
packages/sqlite-web/src/db.rs (1)
new(42-52)packages/sqlite-web/src/errors.rs (3)
from(18-20)from(24-26)from(30-35)
packages/sqlite-web-core/src/worker.rs (1)
packages/sqlite-web-core/src/coordination.rs (1)
new(72-121)
packages/sqlite-web/src/params.rs (1)
packages/sqlite-web/src/errors.rs (3)
from(18-20)from(24-26)from(30-35)
packages/sqlite-web/src/tests.rs (7)
packages/sqlite-web/src/worker.rs (2)
handle_worker_control_message(57-74)serde_wasm_bindgen(58-58)packages/sqlite-web/src/worker_template.rs (1)
generate_self_contained_worker(4-14)packages/sqlite-web-core/src/coordination.rs (1)
js_sys(400-401)packages/sqlite-web/src/db.rs (1)
new(42-52)packages/sqlite-web/src/ready.rs (2)
new(25-40)current_state(42-44)packages/sqlite-web/src/errors.rs (3)
from(18-20)from(24-26)from(30-35)packages/sqlite-web/src/params.rs (3)
v(32-32)v(35-35)v(38-38)
packages/sqlite-web/src/db.rs (6)
packages/sqlite-web/src/params.rs (1)
normalize_params_js(7-14)packages/sqlite-web/src/utils.rs (1)
describe_js_value(3-14)packages/sqlite-web/src/worker.rs (2)
create_worker_from_code(15-27)install_onmessage_handler(29-46)packages/sqlite-web/src/worker_template.rs (1)
generate_self_contained_worker(4-14)packages/sqlite-web/src/ready.rs (1)
new(25-40)packages/sqlite-web/src/errors.rs (3)
from(18-20)from(24-26)from(30-35)
packages/sqlite-web/src/worker.rs (2)
packages/sqlite-web/src/utils.rs (1)
describe_js_value(3-14)packages/sqlite-web/src/ready.rs (2)
new(25-40)current_state(42-44)
packages/sqlite-web-core/src/coordination.rs (2)
packages/sqlite-web-core/src/util.rs (3)
js_value_to_string(39-60)set_js_property(29-37)sanitize_identifier(5-19)packages/sqlite-web-core/src/worker.rs (1)
send_worker_error_message(19-30)
🔇 Additional comments (16)
packages/sqlite-web-core/src/messages.rs (1)
29-33: Leader ping variant matches the schema.Serde rename keeps the payload aligned with existing channel events, and the roundtrip test locks the JSON shape.
packages/sqlite-web/src/worker_template.rs (2)
7-10: Timeout constant injection looks solid.Encoding the DB name first and then defining
__SQLITE_FOLLOWER_TIMEOUT_MSkeeps the worker bootstrap self-contained while giving followers the shared budget.
23-44: WASM tests cover the new worker prefix.These browser tests pin the prefix values and ensure the embedded worker stays untouched, catching regressions early.
packages/sqlite-web/src/utils.rs (2)
3-14: Helper captures the common JS value cases cleanly.String/number fast paths and debug fallback give concise diagnostics without extra allocations.
23-42: Nice WASM-side assertions.The browser tests walk through strings, integers, floats, and objects, so the helper’s behavior is fully exercised.
packages/sqlite-web/src/lib.rs (1)
9-10: Re-exports keep the public API intact.Pulling
SQLiteWasmDatabaseandSQLiteWasmDatabaseErrorfrom the new modules maintains existing consumers while letting the internals refactor freely.packages/sqlite-web/src/errors.rs (2)
5-35: Error type covers the wasm surface area.Variant set lines up with serde, JS interop, and initialization flows, and the conversions give consistent messages back to the host.
46-87: Conversion tests are thorough.Round-tripping through
JsValueandWasmEncodedErrorplus the serde variant check should catch regressions in the glue code.packages/sqlite-web/src/db.rs (8)
1-17: LGTM!The imports are well-organized and all necessary for the WASM database interface implementation.
19-25: LGTM!The struct design appropriately uses interior mutability patterns to manage worker communication state and track pending queries.
42-52: LGTM!The constructor properly validates input before constructing resources and correctly separates construction from readiness synchronization.
54-70: LGTM!The internal constructor properly initializes all state and wires up worker communication. Starting request_id at 1 is correct (avoids 0).
72-75: LGTM!The parameter normalization properly converts Rust Option to JS undefined and delegates to the dedicated params module.
120-122: LGTM!The early check for initialization failure prevents unnecessary work and provides a clear error to the caller.
132-137: LGTM!The request ID generation with wrapping and skipping 0 is correct. While theoretically IDs could collide after wrapping (requiring billions of concurrent queries), this is not a practical concern.
187-274: LGTM!The tests provide good coverage of parameter normalization edge cases and input validation. The tests correctly verify BigInt and ArrayBuffer encoding formats.
Comment on lines
+27
to
+36
| impl Serialize for SQLiteWasmDatabase { | ||
| fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> | ||
| where | ||
| S: serde::Serializer, | ||
| { | ||
| use serde::ser::SerializeStruct; | ||
| let state = serializer.serialize_struct("SQLiteWasmDatabase", 0)?; | ||
| state.end() | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider documenting why Serialize is empty.
The empty Serialize implementation is valid but may confuse readers since it discards all state. Consider adding a comment explaining that the worker and callbacks cannot be serialized and this is intentionally minimal.
🤖 Prompt for AI Agents
In packages/sqlite-web/src/db.rs around lines 27 to 36, the custom Serialize
impl returns an empty struct which silently discards all state; add a short
comment above the impl (or inside the method) stating that this is intentional
because the database contains non-serializable runtime state (worker handles,
callbacks, threads, etc.) and therefore must be serialized as an empty
placeholder, so future readers won’t think this is a bug. Ensure the comment
briefly lists the non-serializable pieces and that any future state intended for
persistence must be explicitly added and documented.
There was a problem hiding this comment.
@findolor Understood, no problem! 👍
🧠 Learnings used
Learnt from: CR
Repo: rainlanguage/sqlite-web PR: 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
Repo: rainlanguage/sqlite-web PR: 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
Repo: rainlanguage/sqlite-web PR: 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
Repo: rainlanguage/sqlite-web PR: 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: findolor
Repo: rainlanguage/sqlite-web PR: 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
Repo: rainlanguage/sqlite-web PR: 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
Repo: rainlanguage/sqlite-web PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web/embedded_worker.js : Do not manually edit embedded_worker.js; it is generated by the build and should be treated as read-only
Learnt from: findolor
Repo: rainlanguage/sqlite-web PR: 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.
Learnt from: CR
Repo: rainlanguage/sqlite-web PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Maintain self-contained workers with no external WASM file dependencies across the project
Learnt from: findolor
Repo: rainlanguage/sqlite-web PR: 14
File: svelte-test/tests/database-functions/float-is-zero.test.ts:39-57
Timestamp: 2025-09-19T07:06:20.173Z
Learning: PRAGMA function_list is not available in the WASM SQLite test environment used by svelte-test, so availability testing should rely on direct function calls rather than PRAGMA queries.
Comment on lines
110
to
184
| pub async fn query( | ||
| &self, | ||
| sql: &str, | ||
| params: Option<Array>, | ||
| ) -> Result<String, SQLiteWasmDatabaseError> { | ||
| let worker = &self.worker; | ||
| let pending_queries = Rc::clone(&self.pending_queries); | ||
| let sql = sql.to_string(); | ||
| let params_array = Self::normalize_params(params)?; | ||
|
|
||
| if let InitializationState::Failed(reason) = self.ready_signal.current_state() { | ||
| return Err(SQLiteWasmDatabaseError::InitializationFailed(reason)); | ||
| } | ||
|
|
||
| let message = js_sys::Object::new(); | ||
| js_sys::Reflect::set( | ||
| &message, | ||
| &JsValue::from_str("type"), | ||
| &JsValue::from_str("execute-query"), | ||
| ) | ||
| .map_err(SQLiteWasmDatabaseError::JsError)?; | ||
|
|
||
| let request_id = { | ||
| let mut n = self.next_request_id.borrow_mut(); | ||
| let id = *n; | ||
| *n = n.wrapping_add(1).max(1); | ||
| id | ||
| }; | ||
| js_sys::Reflect::set( | ||
| &message, | ||
| &JsValue::from_str("requestId"), | ||
| &JsValue::from_f64(request_id as f64), | ||
| ) | ||
| .map_err(SQLiteWasmDatabaseError::JsError)?; | ||
| js_sys::Reflect::set( | ||
| &message, | ||
| &JsValue::from_str("sql"), | ||
| &JsValue::from_str(&sql), | ||
| ) | ||
| .map_err(SQLiteWasmDatabaseError::JsError)?; | ||
| if params_array.length() > 0 { | ||
| let params_js = JsValue::from(params_array.clone()); | ||
| js_sys::Reflect::set(&message, &JsValue::from_str("params"), ¶ms_js) | ||
| .map_err(SQLiteWasmDatabaseError::JsError)?; | ||
| } | ||
|
|
||
| let rid_for_insert = request_id; | ||
| let promise = | ||
| js_sys::Promise::new(&mut |resolve, reject| match worker.post_message(&message) { | ||
| Ok(()) => { | ||
| pending_queries | ||
| .borrow_mut() | ||
| .insert(rid_for_insert, (resolve, reject)); | ||
| } | ||
| Err(err) => { | ||
| let _ = reject.call1(&JsValue::NULL, &err); | ||
| } | ||
| }); | ||
|
|
||
| let result = match JsFuture::from(promise).await { | ||
| Ok(value) => value, | ||
| Err(err) => { | ||
| if err | ||
| .as_string() | ||
| .as_deref() | ||
| .map(|s| s == "InitializationPending") | ||
| .unwrap_or(false) | ||
| { | ||
| return Err(SQLiteWasmDatabaseError::InitializationPending); | ||
| } | ||
| return Err(SQLiteWasmDatabaseError::JsError(err)); | ||
| } | ||
| }; | ||
| Ok(result.as_string().unwrap_or_else(|| format!("{result:?}"))) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Query execution logic is correct; consider future resilience improvements.
The query method correctly constructs messages, posts them to the worker, and awaits results. However, consider the following optional improvements for production resilience:
- Per-query timeout: Queries currently wait indefinitely for the worker. A configurable timeout per query would prevent indefinite hangs.
- Pending queries cleanup: If the worker crashes or queries are abandoned, entries in
pending_queriesare never removed, leading to a memory leak. - Worker lifecycle management: Consider adding a
close()orterminate()method to explicitly shut down the worker when the database is no longer needed.
There was a problem hiding this comment.
@findolor Understood, thanks for the clarification!
🧠 Learnings used
Learnt from: CR
Repo: rainlanguage/sqlite-web PR: 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
Repo: rainlanguage/sqlite-web PR: 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
Repo: rainlanguage/sqlite-web PR: 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: findolor
Repo: rainlanguage/sqlite-web PR: 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
Repo: rainlanguage/sqlite-web PR: 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
Repo: rainlanguage/sqlite-web PR: 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
Repo: rainlanguage/sqlite-web PR: 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.
Learnt from: findolor
Repo: rainlanguage/sqlite-web PR: 15
File: svelte-test/tests/integration/error-handling.test.ts:418-434
Timestamp: 2025-09-17T17:44:54.855Z
Learning: In the sqlite-web test harness, parameter binding is not available for the db.query() method, so string interpolation with manual SQL escaping using value.replace(/'/g, "''") is the necessary approach for handling special characters in test data.
Learnt from: CR
Repo: rainlanguage/sqlite-web PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web/embedded_worker.js : Do not manually edit embedded_worker.js; it is generated by the build and should be treated as read-only
Learnt from: CR
Repo: rainlanguage/sqlite-web PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Maintain self-contained workers with no external WASM file dependencies across the project
Learnt from: findolor
Repo: rainlanguage/sqlite-web PR: 14
File: svelte-test/tests/database-functions/float-is-zero.test.ts:39-57
Timestamp: 2025-09-19T07:06:20.173Z
Learning: PRAGMA function_list is not available in the WASM SQLite test environment used by svelte-test, so availability testing should rely on direct function calls rather than PRAGMA queries.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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 (8)
packages/sqlite-web-core/src/coordination.rs(13 hunks)packages/sqlite-web-core/src/messages.rs(5 hunks)packages/sqlite-web-core/src/worker.rs(5 hunks)packages/sqlite-web/src/db.rs(1 hunks)packages/sqlite-web/src/lib.rs(1 hunks)packages/sqlite-web/src/messages.rs(1 hunks)packages/sqlite-web/src/ready.rs(1 hunks)packages/sqlite-web/src/worker.rs(1 hunks)
🧰 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 (14)
📓 Common learnings
Learnt from: CR
Repo: rainlanguage/sqlite-web PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Maintain self-contained workers with no external WASM file dependencies across the project
Learnt from: CR
Repo: rainlanguage/sqlite-web PR: 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
Repo: rainlanguage/sqlite-web PR: 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
Repo: rainlanguage/sqlite-web PR: 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
Repo: rainlanguage/sqlite-web PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web/embedded_worker.js : Do not manually edit embedded_worker.js; it is generated by the build and should be treated as read-only
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
Repo: rainlanguage/sqlite-web PR: 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/worker.rspackages/sqlite-web/src/ready.rspackages/sqlite-web-core/src/coordination.rspackages/sqlite-web/src/messages.rspackages/sqlite-web-core/src/worker.rspackages/sqlite-web/src/db.rspackages/sqlite-web/src/lib.rspackages/sqlite-web-core/src/messages.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
Repo: rainlanguage/sqlite-web PR: 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/worker.rspackages/sqlite-web/src/ready.rspackages/sqlite-web-core/src/coordination.rspackages/sqlite-web/src/messages.rspackages/sqlite-web-core/src/worker.rspackages/sqlite-web/src/db.rspackages/sqlite-web/src/lib.rspackages/sqlite-web-core/src/messages.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
Repo: rainlanguage/sqlite-web PR: 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/worker.rspackages/sqlite-web/src/ready.rspackages/sqlite-web-core/src/coordination.rspackages/sqlite-web/src/messages.rspackages/sqlite-web-core/src/worker.rspackages/sqlite-web/src/db.rspackages/sqlite-web/src/lib.rspackages/sqlite-web-core/src/messages.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
Repo: rainlanguage/sqlite-web PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Maintain self-contained workers with no external WASM file dependencies across the project
Applied to files:
packages/sqlite-web/src/worker.rspackages/sqlite-web-core/src/coordination.rspackages/sqlite-web-core/src/worker.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
Repo: rainlanguage/sqlite-web PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web/embedded_worker.js : Do not manually edit embedded_worker.js; it is generated by the build and should be treated as read-only
Applied to files:
packages/sqlite-web/src/worker.rspackages/sqlite-web-core/src/coordination.rspackages/sqlite-web/src/messages.rspackages/sqlite-web-core/src/worker.rspackages/sqlite-web/src/db.rspackages/sqlite-web-core/src/messages.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
Repo: rainlanguage/sqlite-web PR: 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/worker.rspackages/sqlite-web/src/ready.rspackages/sqlite-web-core/src/coordination.rspackages/sqlite-web/src/messages.rspackages/sqlite-web/src/db.rspackages/sqlite-web/src/lib.rspackages/sqlite-web-core/src/messages.rs
📚 Learning: 2025-09-15T06:11:31.781Z
Learnt from: findolor
Repo: rainlanguage/sqlite-web PR: 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/src/ready.rspackages/sqlite-web-core/src/coordination.rspackages/sqlite-web/src/db.rspackages/sqlite-web/src/lib.rs
📚 Learning: 2025-09-17T06:55:20.178Z
Learnt from: CR
Repo: rainlanguage/sqlite-web PR: 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/coordination.rspackages/sqlite-web/src/db.rspackages/sqlite-web/src/lib.rs
📚 Learning: 2025-10-20T12:24:07.874Z
Learnt from: findolor
Repo: rainlanguage/sqlite-web PR: 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/src/db.rspackages/sqlite-web/src/lib.rs
📚 Learning: 2025-09-19T07:06:20.173Z
Learnt from: findolor
Repo: rainlanguage/sqlite-web PR: 14
File: svelte-test/tests/database-functions/float-is-zero.test.ts:39-57
Timestamp: 2025-09-19T07:06:20.173Z
Learning: PRAGMA function_list is not available in the WASM SQLite test environment used by svelte-test, so availability testing should rely on direct function calls rather than PRAGMA queries.
Applied to files:
packages/sqlite-web/src/db.rspackages/sqlite-web/src/lib.rs
📚 Learning: 2025-08-27T05:55:40.481Z
Learnt from: findolor
Repo: rainlanguage/sqlite-web PR: 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/src/db.rs
📚 Learning: 2025-09-17T17:44:54.855Z
Learnt from: findolor
Repo: rainlanguage/sqlite-web PR: 15
File: svelte-test/tests/integration/error-handling.test.ts:418-434
Timestamp: 2025-09-17T17:44:54.855Z
Learning: In the sqlite-web test harness, parameter binding is not available for the db.query() method, so string interpolation with manual SQL escaping using value.replace(/'/g, "''") is the necessary approach for handling special characters in test data.
Applied to files:
packages/sqlite-web/src/lib.rs
📚 Learning: 2025-09-17T08:04:44.062Z
Learnt from: findolor
Repo: rainlanguage/sqlite-web PR: 6
File: svelte-test/tests/database-functions/float-sum.test.ts:78-86
Timestamp: 2025-09-17T08:04:44.062Z
Learning: In svelte-test/tests/database-functions/float-sum.test.ts, the schema defines `amount TEXT NOT NULL` but the NULL value handling tests attempt to insert NULL values, creating a constraint violation that prevents the tests from passing.
Applied to files:
packages/sqlite-web/src/lib.rs
🧬 Code graph analysis (5)
packages/sqlite-web/src/worker.rs (2)
packages/sqlite-web/src/utils.rs (1)
describe_js_value(3-14)packages/sqlite-web/src/ready.rs (2)
new(25-40)current_state(42-44)
packages/sqlite-web/src/ready.rs (3)
packages/sqlite-web-core/src/coordination.rs (2)
js_sys(411-412)new(72-121)packages/sqlite-web/src/db.rs (1)
new(43-53)packages/sqlite-web/src/errors.rs (3)
from(18-20)from(24-26)from(30-35)
packages/sqlite-web-core/src/coordination.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/worker.rs (1)
send_worker_error_message(21-32)
packages/sqlite-web-core/src/worker.rs (2)
packages/sqlite-web-core/src/coordination.rs (2)
js_sys(411-412)new(72-121)packages/sqlite-web-core/src/util.rs (1)
set_js_property(29-37)
packages/sqlite-web/src/db.rs (6)
packages/sqlite-web/src/params.rs (1)
normalize_params_js(7-14)packages/sqlite-web/src/utils.rs (1)
describe_js_value(3-14)packages/sqlite-web/src/worker.rs (2)
create_worker_from_code(14-26)install_onmessage_handler(28-45)packages/sqlite-web/src/worker_template.rs (1)
generate_self_contained_worker(4-14)packages/sqlite-web/src/ready.rs (1)
new(25-40)packages/sqlite-web/src/errors.rs (3)
from(18-20)from(24-26)from(30-35)
⏰ 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 (13)
packages/sqlite-web/src/lib.rs (1)
1-14: LGTM - Clean modularization.The refactoring organizes the codebase into well-defined modules while preserving the public API surface through re-exports. The module boundaries are clear and follow Rust conventions.
packages/sqlite-web-core/src/worker.rs (5)
11-13: LGTM - Import aligns with structured error handling.The addition of
WorkerMessageand error type constants fromcrate::messagessupports the new structured error payload system introduced in this PR.
50-64: LGTM - Well-structured error helper.The
make_structured_errorfunction correctly creates typed error objects that align with theWorkerErrorPayloadstructure. The conditional logic appropriately distinguishes between initialization-pending and generic error types.
84-85: LGTM - Upgraded to structured error format.The query result error handling now uses
make_structured_error, ensuring error responses include bothtypeandmessagefields. This improves error categorization and client-side error handling.
149-155: LGTM - Enhanced initialization error visibility.Adding
inspect_errto propagate initialization errors viasend_worker_errorimproves debugging without altering the error propagation flow. Errors are now visible to the host during worker setup failures.
169-169: LGTM - Leader probe initialization.The call to
state.start_leader_probe()implements the follower timeout feature described in the PR objectives, enabling workers to detect leaders quickly and fail fast when deadlines expire.packages/sqlite-web-core/src/messages.rs (4)
4-5: LGTM - Error type constants for structured messaging.These constants define the error type discriminators used in
WorkerErrorPayload, enabling typed error handling across the worker communication layer.
7-14: LGTM - Well-designed error payload structure.
WorkerErrorPayloadprovides a typed, serializable error structure with appropriate serde attributes for JavaScript interop. The optionalmessagefield withskip_serializing_ifkeeps payloads concise when messages aren't needed.
41-45: LGTM - Leader discovery message type.The
LeaderPingvariant enables the follower timeout mechanism by allowing workers to probe for leader presence. The structure follows established patterns in theChannelMessageenum.
72-72: LGTM - Upgraded to structured error payload.Changing the
errorfield fromOption<String>toOption<WorkerErrorPayload>enables typed error handling and aligns with the broader structured messaging improvements in this PR.packages/sqlite-web/src/ready.rs (3)
9-40: LGTM - Sound readiness coordination design.The
InitializationStateenum andReadySignalstructure provide a clear state machine for worker readiness. The use ofRc<RefCell<>>for interior mutability is appropriate for single-threaded WASM contexts, and the promise-based API integrates well with JavaScript async patterns.
54-75: LGTM - Past review concern resolved.The
mark_readyimplementation now correctly prevents overridingFailedstates by checking the state and returning early (Line 59). The transition and handler cleanup only occur when transitioning fromPendingtoReady, ensuring failure states remain terminal as intended.
77-102: LGTM - Correct failure handling and promise creation.The
mark_failedmethod properly transitions to theFailedstate, invokes the reject callback with the reason, and cleans up handlers. Thecreate_ready_promisehelper correctly captures resolve/reject functions into the provided cells. The implementation is sound and well-tested.
Comment on lines
+1
to
+3
| #[cfg(all(test, target_family = "wasm"))] | ||
| pub const WORKER_ERROR_TYPE_GENERIC: &str = "WorkerError"; | ||
| pub const WORKER_ERROR_TYPE_INITIALIZATION_PENDING: &str = "InitializationPending"; |
There was a problem hiding this comment.
Fix cfg attribute scope - only the first constant is gated.
The #[cfg(all(test, target_family = "wasm"))] attribute on Line 1 applies only to Line 2 (WORKER_ERROR_TYPE_GENERIC). Line 3 (WORKER_ERROR_TYPE_INITIALIZATION_PENDING) is unconditionally compiled, creating an inconsistency where one constant is test-only and the other is always available.
Apply this diff to gate both constants consistently:
-#[cfg(all(test, target_family = "wasm"))]
-pub const WORKER_ERROR_TYPE_GENERIC: &str = "WorkerError";
-pub const WORKER_ERROR_TYPE_INITIALIZATION_PENDING: &str = "InitializationPending";
+#[cfg(all(test, target_family = "wasm"))]
+pub const WORKER_ERROR_TYPE_GENERIC: &str = "WorkerError";
+#[cfg(all(test, target_family = "wasm"))]
+pub const WORKER_ERROR_TYPE_INITIALIZATION_PENDING: &str = "InitializationPending";📝 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.
Suggested change
| #[cfg(all(test, target_family = "wasm"))] | |
| pub const WORKER_ERROR_TYPE_GENERIC: &str = "WorkerError"; | |
| pub const WORKER_ERROR_TYPE_INITIALIZATION_PENDING: &str = "InitializationPending"; | |
| #[cfg(all(test, target_family = "wasm"))] | |
| pub const WORKER_ERROR_TYPE_GENERIC: &str = "WorkerError"; | |
| #[cfg(all(test, target_family = "wasm"))] | |
| pub const WORKER_ERROR_TYPE_INITIALIZATION_PENDING: &str = "InitializationPending"; |
🤖 Prompt for AI Agents
In packages/sqlite-web/src/messages.rs around lines 1 to 3, the #[cfg(all(test,
target_family = "wasm"))] attribute currently only gates
WORKER_ERROR_TYPE_GENERIC (line 2) while
WORKER_ERROR_TYPE_INITIALIZATION_PENDING (line 3) is unconditionally compiled;
make the cfg scope consistent by applying the same #[cfg(all(test, target_family
= "wasm"))] attribute to the second constant as well (or wrap both constants in
a single module or block gated by that cfg) so both constants are compiled under
the same condition.
Collaborator
Author
There was a problem hiding this comment.
this is not an issue
There was a problem hiding this comment.
@findolor, understood! I see now that the different scoping is intentional—WORKER_ERROR_TYPE_GENERIC is test-only while WORKER_ERROR_TYPE_INITIALIZATION_PENDING is used in production code. Thank you for the clarification.
🧠 Learnings used
Learnt from: CR
Repo: rainlanguage/sqlite-web PR: 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
Repo: rainlanguage/sqlite-web PR: 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
Repo: rainlanguage/sqlite-web PR: 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
Repo: rainlanguage/sqlite-web PR: 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
Repo: rainlanguage/sqlite-web PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-17T06:55:20.178Z
Learning: Applies to packages/sqlite-web/embedded_worker.js : Do not manually edit embedded_worker.js; it is generated by the build and should be treated as read-only
Learnt from: CR
Repo: rainlanguage/sqlite-web PR: 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
Repo: rainlanguage/sqlite-web PR: 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: findolor
Repo: rainlanguage/sqlite-web PR: 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.
0xgleb
requested changes
Nov 11, 2025
Comment on lines
334
to
335
| is_leader: &Rc<RefCell<bool>>, | ||
| has_leader: &Rc<RefCell<bool>>, |
There was a problem hiding this comment.
i think it's best to avoid boolean blindness
https://medium.com/@itsme.mittal/boolean-blindness-60937910e40e
Comment on lines
382
to
383
| ChannelMessage::LeaderPing { requester_id: _ } => { | ||
| if *is_leader.borrow() { |
There was a problem hiding this comment.
nitpick: would inlining the if not work?
Suggested change
| ChannelMessage::LeaderPing { requester_id: _ } => { | |
| if *is_leader.borrow() { | |
| ChannelMessage::LeaderPing { requester_id: _ } if *is_leader.borrow() => { |
|
|
||
| let result = match JsFuture::from(promise).await { | ||
| Ok(value) => value, | ||
| Err(err) => { |
There was a problem hiding this comment.
can reduce nesting with if inlining here too
0xgleb
approved these changes
Nov 14, 2025
hardyjosh
approved these changes
Nov 14, 2025
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
See issues:
Solution
has_leaderflag and the configured follower-timeout value, so followers wait only as long as intended before giving upPromise::raceagainst the timeout let followers detect leaders quickly and fail fast when the deadline hitsChecks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit