Add nix and additional commands #1
Conversation
WalkthroughAdds a Nix flake for per-system dev tasks and shells; replaces bun with npm in a bundling script; removes a legacy test runner script. Introduces ESLint config and tooling to svelte-test, tightens types and error handling in Svelte pages, and makes minor Rust test/formatting adjustments without altering public APIs. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Page as Svelte Page (+page.svelte)
participant DB as SQLiteWasmDatabase
participant Storage as OPFS/Browser
User->>Page: Add/Query action
Page->>DB: initialize()
alt init fails
DB-->>Page: { error }
Page-->>User: Show init error
else init ok
DB-->>Page: { value: db }
Page->>DB: execute(sql, params)
alt query fails
DB-->>Page: { error }
Page-->>User: Show query error
else query ok
DB-->>Page: { value: json }
Page->>Page: JSON.parse(value || "[]")
Page-->>User: Render results
end
end
Page->>Page: finally { isLoading/isExecuting = false }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (5)
packages/sqlite-worker/src/lib.rs (2)
123-149: Fix LIFO bug in pending query resolution; use FIFO to preserve request/response orderUsing Vec::pop() makes resolution LIFO; if multiple queries are inflight, responses can resolve the wrong promise. Use VecDeque and pop_front() to ensure FIFO, or (better) correlate with IDs.
Apply this diff to switch to VecDeque FIFO:
@@ -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())); @@ - if let Some((resolve, reject)) = pending_queries_clone.borrow_mut().pop() { + if let Some((resolve, reject)) = pending_queries_clone.borrow_mut().pop_front() {And push to the back when enqueueing:
@@ - // Store the promise callbacks - pending_queries.borrow_mut().push((resolve, reject)); + // Store the promise callbacks (FIFO) + pending_queries.borrow_mut().push_back((resolve, reject));If responses can arrive out-of-order, prefer tagging requests with an ID and using a HashMap instead of a queue.
101-117: Expose a readiness signal instead of relying on arbitrary sleeps downstreamYou parse the “worker-ready” message and early-return, but don’t persist readiness in the instance. Downstream code currently sleeps for 1s to “wait for ready,” which is brittle.
Suggestions:
- Track readiness in SQLiteWasmDatabase (e.g., a oneshot or Promise-like flag) and gate query() until ready.
- Provide a ready() async method users can await.
- Alternatively, buffer messages until ready.
I can sketch an approach if you want this in the same PR.
svelte-test/src/routes/+page.svelte (3)
140-145: Critical: Svelte event handlers must use on:click, not onclickUsing onclick sets a DOM attribute, not a Svelte event directive; handlers won’t fire as intended.
Apply this diff:
- <button - onclick={addUser} + <button + on:click={addUser} disabled={isLoading || !newUserName.trim() || !newUserEmail.trim()} > @@ - <button class="refresh-btn" onclick={loadUsers} disabled={isLoading}> + <button class="refresh-btn" on:click={loadUsers} disabled={isLoading}> @@ - <button class="clear-btn" onclick={clearAll} disabled={isLoading}> + <button class="clear-btn" on:click={clearAll} disabled={isLoading}> @@ - <button - class="delete-btn" - onclick={() => deleteUser(user.id)} + <button + class="delete-btn" + on:click={() => deleteUser(user.id)} disabled={isLoading} > @@ - <button onclick={() => window.location.reload()}>Reload Page</button> + <button on:click={() => window.location.reload()}>Reload Page</button>Also applies to: 152-159, 175-182, 195-196
95-97: Prefer parameter binding for id in DELETE as wellEven though id is typed as number, using parameters is safer and consistent. If binding isn’t available yet, validate/coerce id to an integer before interpolation.
Example with validation (until params exist):
- await db.query(`DELETE FROM users WHERE id = ${id}`); + const safeId = Number.isInteger(id) ? id : parseInt(String(id), 10); + await db.query(`DELETE FROM users WHERE id = ${safeId}`);
53-55: Nit: extra space in function declarationMinor clean-up.
Apply:
- async function loadUsers() { + async function loadUsers() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
flake.lockis excluded by!**/*.locksvelte-test/bun.lockis excluded by!**/*.locksvelte-test/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
flake.nix(1 hunks)packages/sqlite-worker-core/src/coordination.rs(1 hunks)packages/sqlite-worker-core/src/database_functions.rs(1 hunks)packages/sqlite-worker-core/src/lib.rs(1 hunks)packages/sqlite-worker/src/lib.rs(3 hunks)scripts/local-bundle.sh(1 hunks)svelte-test/.eslintignore(1 hunks)svelte-test/eslint.config.js(1 hunks)svelte-test/package.json(2 hunks)svelte-test/src/app.d.ts(1 hunks)svelte-test/src/app.html(1 hunks)svelte-test/src/routes/+page.svelte(8 hunks)svelte-test/src/routes/sql/+page.svelte(5 hunks)svelte-test/tests/test-setup.js(1 hunks)test.sh(0 hunks)
💤 Files with no reviewable changes (1)
- test.sh
🔇 Additional comments (18)
packages/sqlite-worker-core/src/lib.rs (1)
28-28: Formatting-only change to test body — OKConverts the empty test body to a single-line form. No behavior change.
svelte-test/src/app.html (1)
3-10: Whitespace/indentation-only change — OKNo semantic changes to the markup; placeholders remain intact.
svelte-test/src/app.d.ts (1)
4-10: Indentation-only update — OKNo changes to the App namespace contents or exported types.
packages/sqlite-worker/src/lib.rs (3)
265-270: LGTM: test match arm styling tidy-upConverting the Ok arm to an inline empty block improves readability without changing behavior.
279-284: LGTM: test match arm styling tidy-upSame here; formatting-only change looks good.
293-300: LGTM: test match arm styling tidy-upConsistent minimal arms improve signal-to-noise in tests.
svelte-test/tests/test-setup.js (1)
19-20: Good defensive logging for unknown errorsGuarding non-Error values improves resilience in diverse environments.
svelte-test/src/routes/+page.svelte (1)
57-60: Result handling assumes a stringified JSON payloadIf db.query returns a string and not JSON, JSON.parse will throw. Ensure the worker returns JSON for SELECTs, or gate parsing with a try/catch.
If SELECTs return rows JSON from the worker, this is fine. Otherwise, wrap JSON.parse in try/catch and handle non-JSON gracefully. Do you want me to add a robust parser with fallback?
svelte-test/src/routes/sql/+page.svelte (4)
5-11: Good improvements to type safety and error handling.The enhanced typing for
queryResultand comprehensive error message handling throughout the component strengthen the code quality. The use of$statefor reactive state management is appropriate for Svelte 5.
59-59: Good defensive JSON parsing.Using
result.value || '[]'as a fallback ensures the parse operation won't fail on undefined/null values.
65-69: Excellent error handling pattern with finally block.The comprehensive error handling with proper cleanup in the finally block ensures the UI state remains consistent regardless of query execution outcome.
112-112: Good UX improvement with loading state feedback.Showing "Executing..." provides clear feedback to users during query execution.
scripts/local-bundle.sh (1)
125-127: Migration from bun to npm looks correct.The replacement of
buncommands withnpmequivalents maintains the same functionality while standardizing on npm for package management.svelte-test/package.json (4)
34-34: Good change to use explicit test execution mode.Changing from
vitesttovitest runensures tests run once and exit, which is better for CI/CD pipelines and automated testing.
37-39: Useful development scripts added.The addition of
lint,format, andlint-format-checkscripts provides convenient commands for code quality maintenance.
43-43: Correct update to use file: protocol for local packages.The change from a relative path to
file:protocol is the proper way to reference local npm packages.
10-11: No version mismatch in TypeScript ESLint packagesThe
@typescript-eslint/eslint-pluginand@typescript-eslint/parserare both on version 7.18.0, which is correct—they’re released in lockstep and must share the same major/minor version. There is no separate “typescript-eslint” runtime package at 8.14.0, so there’s no compatibility issue here. If you do want to upgrade, bump both plugin and parser together to the same 8.x version (e.g. 8.39.1).Likely an incorrect or invalid review comment.
flake.nix (1)
1-75: Well-structured Nix flake for development workflows.The flake provides a comprehensive development environment with clear task separation and proper dependency management. The use of
rainixfor task definitions and the hierarchical task structure (test-full-integration depending on test-wasm and test-ui) is well-designed.
| } | ||
|
|
||
| let remaining_count = test_queries.len() - (test_queries.len() + 1) / 2; | ||
| let remaining_count = test_queries.len() - test_queries.len().div_ceil(2); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix computes the correct remaining count after removing even-indexed entries
Using len() - len().div_ceil(2) yields floor(n/2), which matches the number of odd-indexed elements remaining after removing indices where i % 2 == 0. Good correction.
One caveat: usize::div_ceil requires a relatively recent Rust toolchain. Verify the MSRV/toolchain pinned in the repo to ensure CI doesn’t break. You can run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Looking for rust-toolchain files..."
fd -HI '^rust-toolchain.*' -a | while read -r f; do
echo "==> $f"
cat "$f"
echo
done
echo "Searching Cargo.toml for rust-version..."
fd -a Cargo.toml | xargs -I{} sh -c 'echo "==> {}"; rg -n "^rust-version\\s*=.*" "{}" || true; echo'
echo "Searching nix flake for rust version/channel hints..."
fd -a flake.nix | xargs -I{} sh -c 'echo "==> {}"; rg -n "rust|toolchain|msrv|rustc" "{}" -n -A2 || true; echo'Length of output: 763
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspecting rust-version in Cargo.toml files ==="
fd -a Cargo.toml | while read -r file; do
echo "==> $file"
rg -n "^rust-version\\s*=" "$file" || echo " (no rust-version field)"
echo
done
echo "=== Scanning flake.nix for Rust toolchain hints ==="
sed -n '1,200p' flake.nix | rg -n "rust|toolchain|msrv|rustc" -n -A2 || echo " (no hints in first 200 lines)"Length of output: 1239
Pin MSRV (Rust 1.60+) for div_ceil support
I didn’t find any explicit toolchain or rust-version pin in the repo (no rust-toolchain* file, no rust-version in any Cargo.toml, no hints in flake.nix). Since usize::div_ceil is only stable from Rust 1.60 onward, please ensure CI and all contributors use at least that version.
• In your root Cargo.toml (and/or each crate that relies on div_ceil), add under [package]:
[package]
name = "…"
version = "…"
+rust-version = "1.60"• Or create a rust-toolchain.toml at the repo root:
[toolchain]
channel = "1.60.0"This will lock in the minimum supported Rust toolchain and prevent unexpected CI failures.
🤖 Prompt for AI Agents
In packages/sqlite-worker-core/src/coordination.rs around line 326, the code
uses usize::div_ceil which is stable only from Rust 1.60+, but the repo has no
pinned toolchain; add a minimum supported Rust version to prevent CI/contributor
breakage by either (a) adding rust-version = "1.60" under [package] in the root
Cargo.toml (and any crates that directly use div_ceil) or (b) adding a
rust-toolchain.toml at the repo root with the toolchain channel set to 1.60.0,
and ensure CI configs are updated to use the same toolchain version.
There was a problem hiding this comment.
rainix, our main input to the nix flake, sets the toolchain version, however, specifying MSRV wouldn't hurt
There was a problem hiding this comment.
This is fine i think we don't need to add it. We either get ci error or not so we can handle it later on
There was a problem hiding this comment.
@findolor Understood! You're right that it's not strictly necessary if your development environment and CI are already using a recent Rust version. The div_ceil fix itself is solid and will work correctly with the toolchain you're using.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| Some(std::mem::transmute::< | ||
| isize, | ||
| unsafe extern "C" fn(*mut std::ffi::c_void), | ||
| >(-1isize)), // SQLITE_TRANSIENT |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Minor readability nit: factor out the SQLITE_TRANSIENT sentinel
The transmute-based sentinel is correct, but inlining it obscures intent. Consider a small helper to name the sentinel and avoid repeating the transmute expression inline.
Apply this localized change:
- Some(std::mem::transmute::<
- isize,
- unsafe extern "C" fn(*mut std::ffi::c_void),
- >(-1isize)), // SQLITE_TRANSIENT
+ sqlite_transient(), // SQLITE_TRANSIENTAnd add this helper function somewhere near the top of the file (outside the selected lines):
#[inline]
fn sqlite_transient() -> Option<unsafe extern "C" fn(*mut std::ffi::c_void)> {
// Matches SQLite's (sqlite3_destructor_type)(-1) sentinel.
Some(unsafe {
std::mem::transmute::<isize, unsafe extern "C" fn(*mut std::ffi::c_void)>(-1isize)
})
}🤖 Prompt for AI Agents
In packages/sqlite-worker-core/src/database_functions.rs around lines 102 to
105, the inline transmute used to produce the SQLITE_TRANSIENT sentinel should
be replaced with a named helper to improve readability; add a small #[inline]
helper function near the top of the file (outside the selected lines) named
sqlite_transient that returns Option<unsafe extern "C" fn(*mut
std::ffi::c_void)> and performs the transmute to produce the sentinel, then
replace the current Some(std::mem::transmute::<isize, unsafe extern "C" fn(*mut
std::ffi::c_void)>(-1isize)) expression with a call to sqlite_transient() so the
intent is clear and the transmute is not repeated inline.
There was a problem hiding this comment.
This is fine, we don't need to update it
There was a problem hiding this comment.
@findolor Understood! I'll leave the current implementation as-is. The transmute expression works correctly for the SQLITE_TRANSIENT sentinel, and if you prefer keeping it inline, that's perfectly fine.
| .DS_Store | ||
| node_modules | ||
| /build | ||
| /.svelte-kit | ||
| /package | ||
| .env | ||
| .env.* | ||
| !.env.example | ||
|
|
||
| # Ignore files for PNPM, NPM and YARN | ||
| pnpm-lock.yaml | ||
| package-lock.json | ||
| yarn.lock |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Looks good; consider aligning ignores with flat config and adding a few common artifacts
This ignore set is sensible. With flat config, eslint.config.js “ignores” typically supersedes .eslintignore. Consider adding or aligning:
- coverage/
- dist/ (already ignored in config, optional here)
- .vite/ or node_modules/.vite/
- .turbo/, .pnpm-store
- result (Nix build output, if applicable)
Example additions:
.env.*
!.env.example
+
+# Common build/test artifacts
+coverage
+.vite
+.turbo
+result📝 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.
| .DS_Store | |
| node_modules | |
| /build | |
| /.svelte-kit | |
| /package | |
| .env | |
| .env.* | |
| !.env.example | |
| # Ignore files for PNPM, NPM and YARN | |
| pnpm-lock.yaml | |
| package-lock.json | |
| yarn.lock | |
| .DS_Store | |
| node_modules | |
| /build | |
| /.svelte-kit | |
| /package | |
| .env | |
| .env.* | |
| !.env.example | |
| # Common build/test artifacts | |
| coverage | |
| .vite | |
| .turbo | |
| result | |
| # Ignore files for PNPM, NPM and YARN | |
| pnpm-lock.yaml | |
| package-lock.json | |
| yarn.lock |
🤖 Prompt for AI Agents
In svelte-test/.eslintignore around lines 1 to 13, the current ignore list is
fine but should be aligned with the repository’s flat ESLint config and extended
to cover common build/test artifacts; update the file to include entries such as
coverage/, dist/ (if not already ignored in config), .vite/ and
node_modules/.vite/, .turbo/, .pnpm-store, and result (Nix output) so ESLint and
editors consistently skip generated files and caches across environments.
There was a problem hiding this comment.
This is not needed
There was a problem hiding this comment.
@findolor Understood! The current .eslintignore file covers the essential patterns for your Svelte project setup. Thanks for the clarification.
| ignores: ['build/', '.svelte-kit/', 'dist/'] | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Unify ignores with .eslintignore or consolidate here (flat config prefers in-config ignores)
Flat config setups typically rely on the “ignores” field rather than .eslintignore. Consider adding the same patterns from .eslintignore here (e.g., node_modules, package, env files) or dropping .eslintignore.
Proposed addition:
- ignores: ['build/', '.svelte-kit/', 'dist/']
+ ignores: [
+ 'build/', '.svelte-kit/', 'dist/',
+ 'node_modules/', 'package/',
+ '.env', '.env.*', '!/.env.example'
+ ]📝 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.
| ignores: ['build/', '.svelte-kit/', 'dist/'] | |
| } | |
| ignores: [ | |
| 'build/', '.svelte-kit/', 'dist/', | |
| 'node_modules/', 'package/', | |
| '.env', '.env.*', '!/.env.example' | |
| ] | |
| } |
| status = 'Waiting for worker to be ready...'; | ||
| // Wait for worker to be ready | ||
| await new Promise(resolve => setTimeout(resolve, 1000)); | ||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid fixed timeouts for readiness; poll or handshake
Sleeping for 1s assumes worker readiness; on slow/fast environments this is brittle. If you can’t expose readiness from the Rust side yet, poll with a lightweight SELECT 1 and a timeout, or retry db.query on failure.
I can provide a small waitForReady helper that retries db.query('SELECT 1') with backoff; want me to add it?
🤖 Prompt for AI Agents
In svelte-test/src/routes/+page.svelte around lines 29 to 32, replace the fixed
1s sleep used to wait for the worker with a poll/handshake that retries a
lightweight db.query('SELECT 1') until success or a configurable timeout:
implement a waitForReady helper that attempts db.query('SELECT 1') with
exponential backoff (e.g., start 100ms, double up to a max interval) and an
overall timeout (e.g., 10s), update the status messages during retries, and
throw or show an error if the timeout is reached; use this helper in place of
the fixed await new Promise(resolve => setTimeout(resolve, 1000)).
There was a problem hiding this comment.
This is only used for testing. It's not an issue
There was a problem hiding this comment.
@findolor, understood! You're absolutely right that fixed timeouts are perfectly acceptable in test/demo code where simplicity and predictability are more important than handling all edge cases. I'll keep that context in mind for future reviews.
(\ /)
( ._. )
o_(")(")
learning noted
✏️ Learnings added
Learnt from: findolor
PR: rainlanguage/sqlite-wasm-rs-test#1
File: svelte-test/src/routes/+page.svelte:29-32
Timestamp: 2025-08-18T14:34:35.194Z
Learning: For test and demo code in this repository, fixed timeouts are acceptable and preferred over more complex polling/retry mechanisms for simplicity.
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.
| await db.query(` | ||
| INSERT OR IGNORE INTO users (name, email) | ||
| VALUES ('${newUserName.trim()}', '${newUserEmail.trim()}') | ||
| `); | ||
There was a problem hiding this comment.
SQL injection risk: avoid interpolating untrusted input into SQL strings
Directly embedding user-provided name/email allows crafted inputs to break statements. Prefer parameterized queries or at least escape quotes.
If parameter binding is available, switch to it. If not yet supported, minimally escape single quotes:
- await db.query(`
- INSERT OR IGNORE INTO users (name, email)
- VALUES ('${newUserName.trim()}', '${newUserEmail.trim()}')
- `);
+ const safeName = escapeSql(newUserName.trim());
+ const safeEmail = escapeSql(newUserEmail.trim());
+ await db.query(`
+ INSERT OR IGNORE INTO users (name, email)
+ VALUES ('${safeName}', '${safeEmail}')
+ `);Add this helper near the top of the <script> block:
function escapeSql(value: string): string {
// Basic escaping: double any single quotes
return value.replaceAll("'", "''");
}Longer-term, I recommend adding parameter support in SQLiteWasmDatabase.query (e.g., send a params array alongside sql and bind in the worker) to eliminate this class of bugs end-to-end.
🤖 Prompt for AI Agents
In svelte-test/src/routes/+page.svelte around lines 72 to 76 the SQL is built by
directly interpolating user input (newUserName/newUserEmail) which opens an SQL
injection risk; fix by avoiding raw interpolation — either switch to a
parameterized query API if SQLiteWasmDatabase.query supports params (call
db.query(sql, [name, email]) and bind values), or if params are not yet
available add a small escape helper at the top of the <script> block that
doubles single quotes and use it when composing the INSERT (trim the inputs
first, then escape) so the VALUES clause contains escaped strings rather than
raw user input.
There was a problem hiding this comment.
This is fine as this is only used for testing
There was a problem hiding this comment.
@findolor, I understand your perspective that this is test code. You're right that the immediate security risk is lower in a testing context.
While test code can sometimes become the foundation for production implementations or serve as examples for others, I respect your decision to keep the current approach for this demo. Thanks for the clarification!
✏️ Learnings added
Learnt from: findolor
PR: rainlanguage/sqlite-wasm-rs-test#1
File: svelte-test/src/routes/+page.svelte:72-76
Timestamp: 2025-08-18T14:34:49.608Z
Learning: In the sqlite-wasm-rs-test repository, the maintainer findolor prefers to keep SQL injection vulnerabilities in test/demo code as acceptable since it's only used for testing purposes.
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.
| status = 'Creating database connection...'; | ||
| let res = SQLiteWasmDatabase.new(); | ||
| if (res.error) { | ||
| status = `Failed to create database connection: ${res.error.msg}`; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using a more descriptive error format.
The error message format could be enhanced to include both user-friendly text and technical details.
- status = `Failed to create database connection: ${res.error.msg}`;
+ status = `Failed to create database connection: ${res.error.msg || res.error}`;📝 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.
| status = `Failed to create database connection: ${res.error.msg}`; | |
| status = `Failed to create database connection: ${res.error.msg || res.error}`; |
🤖 Prompt for AI Agents
In svelte-test/src/routes/sql/+page.svelte around line 24, the status assignment
uses only res.error.msg which is not descriptive; update it to build a richer
message that combines a user-facing sentence and the technical details (e.g.,
"Failed to create database connection" plus the error code/stack/message from
res.error), ensuring you guard for missing fields (fallbacks) and avoid exposing
sensitive info by including only necessary technical details for debugging.
There was a problem hiding this comment.
Not needed as this is for testing
There was a problem hiding this comment.
@findolor Understood! You're absolutely right - for testing purposes, the current error handling is perfectly adequate. Thanks for the clarification.
(\ /)
( ._.)
o_(")(")
| // @ts-ignore - entries() method exists but may not be in all type definitions | ||
| for await (const [name, handle] of opfsRoot.entries()) { | ||
| if (name.includes('worker.db') || name.includes('sqlite')) { | ||
| await opfsRoot.removeEntry(name, { recursive: true }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid ts-ignore by guarding entries() at runtime
Instead of suppressing types, check for the method at runtime before iterating.
Apply this diff:
- // @ts-ignore - entries() method exists but may not be in all type definitions
- for await (const [name, handle] of opfsRoot.entries()) {
+ if (typeof opfsRoot.entries === 'function') {
+ for await (const [name, handle] of opfsRoot.entries()) {
if (name.includes('worker.db') || name.includes('sqlite')) {
await opfsRoot.removeEntry(name, { recursive: true });
}
- }
+ }
+ }📝 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.
| // @ts-ignore - entries() method exists but may not be in all type definitions | |
| for await (const [name, handle] of opfsRoot.entries()) { | |
| if (name.includes('worker.db') || name.includes('sqlite')) { | |
| await opfsRoot.removeEntry(name, { recursive: true }); | |
| } | |
| } | |
| if (typeof opfsRoot.entries === 'function') { | |
| for await (const [name, handle] of opfsRoot.entries()) { | |
| if (name.includes('worker.db') || name.includes('sqlite')) { | |
| await opfsRoot.removeEntry(name, { recursive: true }); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In svelte-test/tests/test-setup.js around lines 11 to 16, the code uses //
@ts-ignore and unconditionally iterates opfsRoot.entries(); remove the ts-ignore
and guard the call at runtime by checking that opfsRoot.entries exists and is a
function before using it (e.g. if (opfsRoot && typeof opfsRoot.entries ===
'function') { for await (const [name, handle] of opfsRoot.entries()) { ... } }),
so the loop only runs when entries() is available and no type-suppression
comment is needed.
There was a problem hiding this comment.
This is only used for testing not needed
There was a problem hiding this comment.
@findolor I understand. You're right that test utilities can have more relaxed code quality standards, and the @ts-ignore approach is perfectly acceptable for this testing scenario. I'll note your preference going forward.
🐰
∩ ∩
( .. )
o(")(")
✏️ Learnings added
Learnt from: findolor
PR: rainlanguage/sqlite-wasm-rs-test#1
File: svelte-test/tests/test-setup.js:11-16
Timestamp: 2025-08-18T14:35:41.264Z
Learning: The user findolor prefers to use ts-ignore in test code rather than runtime guards for TypeScript compatibility issues, as test code doesn't need the same quality standards as production code.
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.
| } | ||
|
|
||
| let remaining_count = test_queries.len() - (test_queries.len() + 1) / 2; | ||
| let remaining_count = test_queries.len() - test_queries.len().div_ceil(2); |
There was a problem hiding this comment.
rainix, our main input to the nix flake, sets the toolchain version, however, specifying MSRV wouldn't hurt
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
Bug Fixes
Tests
Chores