-
Notifications
You must be signed in to change notification settings - Fork 0
Add BIGINT_SUM aggregate function with I256 support for handling negative values #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughIntroduces sqlite-web replacing sqlite-worker across manifests, scripts, docs, and Svelte test app. Adds new Rust database functions (BIGINT_SUM, RAIN_MATH_PROCESS) with registration. Adds GitHub Actions workflows for WASM tests, UI tests, and automated npm prerelease publishing. Updates Nix flake with build-submodules task and related build/test targets. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub Actions
participant Nix as Nix Env
participant NPM as npm Registry
participant Repo as Git Repo
Dev->>GH: Push to main
GH->>Repo: Checkout (submodules)
GH->>Nix: Install & restore cache
GH->>Nix: build-submodules, local-bundle, tests
GH->>NPM: View @rainlanguage/sqlite-web latest dist.shasum (OLD_HASH)
GH->>Nix: npm pack local bundle (NEW_HASH)
alt Hash differs
GH->>Repo: Version prerelease (NEW_VERSION), commit+tag
GH->>Repo: Push commit and tag
GH->>NPM: Publish tarball (public)
GH->>Repo: Create GitHub Release with tarball
else No change
GH-->>Dev: Skip publish
end
sequenceDiagram
autonumber
participant App as App/Host
participant SQLite as SQLite Engine
participant FFI as Custom Functions (sqlite-web-core)
App->>SQLite: Open DB handle
App->>FFI: register_custom_functions(db*)
FFI->>SQLite: Register RAIN_MATH_PROCESS (scalar, 2 args)
FFI->>SQLite: Register BIGINT_SUM (aggregate, 1 arg)
App->>SQLite: Execute queries using functions
SQLite->>FFI: Call rain_math_process / bigint_sum_step/final
FFI-->>SQLite: Return results/errors
SQLite-->>App: Query results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 41
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (13)
packages/sqlite-web-core/Cargo.toml (2)
6-8: Do we need cdylib for the core crate? Prefer rlib unless you ship a second WASM.
Building the core as cdylib can increase compile time/size and complicate linking. If only the top-level web crate is the WASM boundary, keep core as the default rlib.[lib] -crate-type = ["cdylib"] +# Use the default 'rlib' for the core crate unless this crate is itself consumed as a WASM/C-ABI artifact. +# crate-type = ["cdylib"]
20-22: Ensure Git submodules are initialized in CI
Therain-math-floatdependency inpackages/sqlite-web-core/Cargo.tomlpoints at../../lib/rain.math.float/crates/float, which is a Git submodule. In a fresh checkout (e.g. GitHub Actions’ defaultactions/checkout), that directory will be missing and the build will fail.Please update your CI workflow (e.g.
.github/workflows/ci.yml) to initialize submodules before building:• Option A: Enable recursive submodules in checkout
- name: Checkout code uses: actions/checkout@v3 with: submodules: recursive• Option B: Explicitly update submodules
- run: git submodule update --init --recursiveAlternatively, if you rely on your Flake’s
build-submodulesstep (as in PR #2), ensure the workflow invokes it (for example,nix build .#build-submodules) prior to building the Rust crates. Let me know if you’d like a proposed GH Actions snippet.CLAUDE.md (1)
34-45: Reflect new database function structure and BIGINT_SUM in architecture docs.Call out the new module organization (database_functions/mod.rs, bigint_sum.rs) and the BIGINT_SUM aggregate using Alloy I256, which is a key PR objective.
- - `database_functions.rs` - Custom database functions (uses rain.math.float) + - `database_functions/` - Module root for custom SQLite functions + - `mod.rs` - registration/wiring + - `bigint_sum.rs` - BIGINT_SUM aggregate using Alloy `I256` (signed 256-bit) + - float functions (via `rain.math.float`)Optionally add a brief example later in the doc:
+### Custom SQLite Functions + +- **BIGINT_SUM(expr TEXT)** — sums signed 256-bit integers represented as strings. + - Accepts negative values; uses Alloy `I256` for correct overflow semantics. + - Example: `SELECT BIGINT_SUM(value_str) FROM ledger;`svelte-test/vitest.config.js (1)
10-21: Harden against path traversal and set default content-type for ‘.mjs’.A crafted request could use absolute/parent paths causing path.join to escape node_modules. Also add a quick MIME for .mjs.
- if (fs.existsSync(fullPath)) { + const normalized = path.normalize(fullPath); + if (!normalized.startsWith(baseDir + path.sep)) { + return next(); + } + if (fs.existsSync(normalized)) { - if (fullPath.endsWith('.wasm')) { + if (normalized.endsWith('.wasm')) { res.setHeader('Content-Type', 'application/wasm'); - } else if (fullPath.endsWith('.js')) { + } else if (normalized.endsWith('.js') || normalized.endsWith('.mjs')) { res.setHeader('Content-Type', 'application/javascript'); } - fs.createReadStream(fullPath).pipe(res); + fs.createReadStream(normalized).pipe(res);Optional: add Cache-Control for deterministic test perf:
+ res.setHeader('Cache-Control', 'public, max-age=0, must-revalidate');svelte-test/src/routes/sql/+page.svelte (3)
29-31: Avoid arbitrary sleeps; use an explicit readiness check.Using setTimeout(1000) is brittle and can flake on CI. Prefer an explicit readiness signal (e.g., a ping query or a ready() API if available).
- status = 'Waiting for worker to be ready...'; - await new Promise(resolve => setTimeout(resolve, 1000)); + status = 'Waiting for worker to be ready...'; + // Prefer an explicit readiness check if available: + // await db.ready?.(); + // Fallback: run a cheap ping query to ensure initialization + await db.query('SELECT 1');
13-46: Consider cleanup on unmount to avoid updating state after destroy and to release worker.If the component is destroyed during async init, subsequent state mutations can warn. Also, terminate/close the worker if the API supports it.
- import { onMount } from 'svelte'; + import { onMount, onDestroy } from 'svelte'; import { browser } from '$app/environment'; import init, { SQLiteWasmDatabase } from 'sqlite-web'; @@ - onMount(async () => { + let destroyed = false; + onDestroy(() => { + destroyed = true; + // If the API exposes cleanup, call it: + db?.terminate?.(); + db?.close?.(); + }); + onMount(async () => { if (!browser) return; try { status = 'Loading SQLite Worker...'; await init(); status = 'Creating database connection...'; let res = SQLiteWasmDatabase.new(); if (res.error) { status = `Failed to create database connection: ${res.error.msg}`; return; } db = res.value; - status = 'Waiting for worker to be ready...'; + if (destroyed) return; + status = 'Waiting for worker to be ready...'; // readiness... @@ - status = 'Setting up database schema...'; + if (destroyed) return; + status = 'Setting up database schema...'; @@ - status = 'Ready ✅'; + if (destroyed) return; + status = 'Ready ✅';Please confirm the correct cleanup method name on SQLiteWasmDatabase (e.g., terminate, close, dispose). If none exists, we should add one in sqlite-web to free worker resources.
55-62: Handle non-JSON query results more explicitly.The fallback to { result } can produce an opaque object. Consider checking for a known shape (rows JSON vs. OK/changes count) to improve UX.
- try { - queryResult = JSON.parse(result.value || '[]'); - } catch { - queryResult = [{ result }]; - } + const text = result.value ?? ''; + try { + const parsed = JSON.parse(text); + queryResult = Array.isArray(parsed) ? parsed : [parsed]; + } catch { + // Fallback: display as a single message row + queryResult = [{ message: text || 'OK' }]; + }svelte-test/src/routes/+page.svelte (4)
52-63: Nit: stray double space in function declaration.Minor style fix.
- async function loadUsers() { + async function loadUsers() {
29-33: Avoid arbitrary sleep; prefer readiness or a ping.Same concern as the SQL console page: waiting 1s is brittle.
- status = 'Waiting for worker to be ready...'; - // Wait for worker to be ready - await new Promise(resolve => setTimeout(resolve, 1000)); + status = 'Waiting for worker to be ready...'; + // Prefer an explicit readiness check if available: + // await db.ready?.(); + await db.query('SELECT 1');
88-99: Same parameterization concern for DELETE.Don’t interpolate id into SQL; bind it.
- await db.query(`DELETE FROM users WHERE id = ${id}`); + await db.query('DELETE FROM users WHERE id = ?', [id]);
101-112: Add defensive cleanup and better error surfacing.Consider closing/terminating the worker on page destroy and showing row counts after mutating operations.
+import { onDestroy } from 'svelte'; @@ onMount(async () => { // ... }); +onDestroy(() => { + db?.terminate?.(); + db?.close?.(); +});svelte-test/README.md (1)
1-16: Update branding: replace “SQLite Worker” with “sqlite-web” throughoutRipgrep shows multiple occurrences of the old name in both documentation and source, which will confuse users if left inconsistent.
Files to update:
- svelte-test/README.md (title, description, features list, prerequisite, usage headings)
- svelte-test/src/routes/+page.svelte
- status message on load
- demo page heading
- svelte-test/src/routes/sql/+page.svelte
- status message on load
Example diffs:
--- a/svelte-test/README.md +++ b/svelte-test/README.md @@ -1,4 +1,4 @@ -# SQLite Worker - Svelte Integration Example +# sqlite-web - Svelte Integration Example -This is a SvelteKit application that demonstrates integration with the SQLite Worker WASM package... +This is a SvelteKit application that demonstrates integration with the sqlite-web WASM package... @@ -7,9 +7,9 @@ -**SQLite Worker Integration** - Uses the parent project's WASM SQLite implementation +**sqlite-web Integration** - Uses the parent project's WASM SQLite implementation -**Local Package Dependency** - Consumes the SQLite worker as a local package +**Local Package Dependency** - Consumes the sqlite-web package as a local dependency -Make sure the parent SQLite Worker project is built: +Make sure the parent sqlite-web project is built: -## SQLite Worker Usage +## sqlite-web Usage -sqlite-web requires: +sqlite-web requires:--- a/svelte-test/src/routes/+page.svelte +++ b/svelte-test/src/routes/+page.svelte @@ -17,7 +17,7 @@ // ... - status = 'Loading SQLite Worker...'; + status = 'Loading sqlite-web...'; // ... @@ -113,7 +113,7 @@ } </script> -<h1>SQLite Worker Demo</h1> +<h1>sqlite-web Demo</h1>--- a/svelte-test/src/routes/sql/+page.svelte +++ b/svelte-test/src/routes/sql/+page.svelte @@ -17,7 +17,7 @@ // ... - status = 'Loading SQLite Worker...'; + status = 'Loading sqlite-web...'; // ...scripts/local-bundle.sh (1)
2-3: Use stricter bash flags (-Eeuo pipefail) and safe IFSPrevents silent failures in pipelines and undefined var usage.
-set -e +set -Eeuo pipefail +IFS=$'\n\t'
♻️ Duplicate comments (1)
svelte-test/tests/integration/worker-communication.test.ts (1)
9-9: Consistent migration to sqlite-web type import.Same comment as in opfs-persistence.test.ts: change is correct; please ensure the repo has no leftover sqlite-worker references.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (4)
.DS_Storeis excluded by!**/.DS_StoreCargo.lockis excluded by!**/*.locksvelte-test/bun.lockis excluded by!**/*.locksvelte-test/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
.github/workflows/npm-release.yaml(1 hunks).github/workflows/test-ui.yaml(1 hunks).github/workflows/test-wasm.yaml(1 hunks).gitignore(1 hunks).vscode/settings.json(1 hunks)CLAUDE.md(4 hunks)Cargo.toml(1 hunks)flake.nix(4 hunks)packages/sqlite-web-core/Cargo.toml(1 hunks)packages/sqlite-web-core/src/database_functions/bigint_sum.rs(1 hunks)packages/sqlite-web-core/src/database_functions/mod.rs(1 hunks)packages/sqlite-web-core/src/database_functions/rain_math.rs(1 hunks)packages/sqlite-web/Cargo.toml(1 hunks)scripts/local-bundle.sh(5 hunks)svelte-test/README.md(2 hunks)svelte-test/eslint.config.js(1 hunks)svelte-test/package.json(1 hunks)svelte-test/src/routes/+page.svelte(2 hunks)svelte-test/src/routes/sql/+page.svelte(1 hunks)svelte-test/tests/database-functions/bigint-sum.test.ts(1 hunks)svelte-test/tests/fixtures/test-helpers.ts(2 hunks)svelte-test/tests/integration/database-basic.test.ts(1 hunks)svelte-test/tests/integration/error-handling.test.ts(1 hunks)svelte-test/tests/integration/opfs-persistence.test.ts(1 hunks)svelte-test/tests/integration/worker-communication.test.ts(1 hunks)svelte-test/vite.config.ts(2 hunks)svelte-test/vitest.config.js(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/src/routes/sql/+page.sveltesvelte-test/eslint.config.jssvelte-test/tests/fixtures/test-helpers.tssvelte-test/tests/integration/database-basic.test.tssvelte-test/vite.config.tssvelte-test/tests/integration/worker-communication.test.tssvelte-test/tests/integration/error-handling.test.tssvelte-test/tests/database-functions/bigint-sum.test.tssvelte-test/vitest.config.jssvelte-test/tests/integration/opfs-persistence.test.tssvelte-test/src/routes/+page.svelte
🧠 Learnings (4)
📓 Common learnings
Learnt from: findolor
PR: rainlanguage/sqlite-web#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.
📚 Learning: 2025-08-21T12:24:20.696Z
Learnt from: findolor
PR: rainlanguage/sqlite-web#2
File: .github/workflows/npm-release.yaml:73-77
Timestamp: 2025-08-21T12:24:20.696Z
Learning: The findolor user prefers to maintain consistency with existing npm-release workflows across repositories, even when technical improvements are available. They use a standardized GitHub Actions workflow pattern for NPM package releases.
Applied to files:
.github/workflows/npm-release.yaml
📚 Learning: 2025-08-21T12:24:24.620Z
Learnt from: findolor
PR: rainlanguage/sqlite-web#2
File: .github/workflows/npm-release.yaml:79-85
Timestamp: 2025-08-21T12:24:24.620Z
Learning: The rainlanguage organization uses workflows that commit pkg/package.json as part of their NPM release process across multiple repositories, and this pattern works fine for their setup.
Applied to files:
.github/workflows/npm-release.yaml
📚 Learning: 2025-08-21T15:09:22.737Z
Learnt from: findolor
PR: rainlanguage/sqlite-web#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:
flake.nix
🪛 LanguageTool
CLAUDE.md
[grammar] ~9-~9: Use correct spacing
Context: ...hat: 1. Builds sqlite-web-core with wasm-pack build --target web 2. Embeds WASM into JavaScript worker templ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~10-~10: There might be a mistake here.
Context: ...eds WASM into JavaScript worker template 3. Builds sqlite-web package with embedde...
(QB_NEW_EN)
[grammar] ~11-~11: There might be a mistake here.
Context: ... sqlite-web package with embedded core 4. Packages with npm pack and updates Sve...
(QB_NEW_EN)
[grammar] ~12-~12: Use correct spacing
Context: ...ackand updates Svelte test integration ### Individual Package Builds -cd packages...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~14-~14: Use correct spacing
Context: ...tegration ### Individual Package Builds - cd packages/sqlite-web-core && wasm-pack build --target web --out-dir ../../pkg - cd packages/sqlite-web && wasm-pack build --target web --out-dir ../../pkg ### Testing - ./test.sh - Run all Rust WAS...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~18-~18: Use correct spacing
Context: ...et web --out-dir ../../pkg ### Testing -./test.sh` - Run all Rust WASM tests (both packages...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~19-~19: Use hyphens correctly
Context: ... Run all Rust WASM tests (both packages) - cd packages/sqlite-web-core && wasm-pack test --headless --chrome - Test core package only - `cd packages/...
(QB_NEW_EN_OTHER_ERROR_IDS_29)
[grammar] ~20-~20: Use hyphens correctly
Context: ...dless --chrome- Test core package only -cd packages/sqlite-web && wasm-pack test --headless --chrome` - Test worker package only ### Svelte T...
(QB_NEW_EN_OTHER_ERROR_IDS_29)
[grammar] ~21-~21: There might be a mistake here.
Context: ...ess --chrome- Test worker package only ### Svelte Test App -cd svelte-test && bun...
(QB_NEW_EN_OTHER)
[grammar] ~34-~34: Use correct spacing
Context: ...lication. ### Core Components #### 1. packages/sqlite-web-core/ - Purpose: Core SQLite functionality and...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~45-~45: Use correct spacing
Context: ... WASM module with JS glue code #### 2. packages/sqlite-web/ - Purpose: Public API that creates self-...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~62-~62: Use correct spacing
Context: ... sqlite-web package from local tarball ### Build Process Flow 1. Core Build: `...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~64-~64: Use correct spacing
Context: ...om local tarball ### Build Process Flow 1. Core Build: sqlite-web-core compiled...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~66-~66: There might be a mistake here.
Context: ...ite-web-core` compiled to WASM + JS glue 2. Embedding: WASM converted to base64 an...
(QB_NEW_EN)
[grammar] ~67-~67: There might be a mistake here.
Context: ...64 and embedded into JavaScript template 3. Wrapper Build: sqlite-web compiled w...
(QB_NEW_EN)
[grammar] ~68-~68: There might be a mistake here.
Context: ... compiled with embedded worker generator 4. Packaging: NPM package created and int...
(QB_NEW_EN)
svelte-test/README.md
[grammar] ~85-~85: Use correct spacing
Context: ...sqlite-web* - Local WASM package from ../pkg/sqlite-web-0.1.0.tgz - @sveltejs/kit - SvelteKit framework - ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
🪛 markdownlint-cli2 (0.17.2)
CLAUDE.md
14-14: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
15-15: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
18-18: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
19-19: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
34-34: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
45-45: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🪛 actionlint (1.7.7)
.github/workflows/test-ui.yaml
17-17: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/test-wasm.yaml
17-17: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/npm-release.yaml
48-48: shellcheck reported issue in this script: SC2086:info:2:30: Double quote to prevent globbing and word splitting
(shellcheck)
55-55: shellcheck reported issue in this script: SC2086:info:3:30: Double quote to prevent globbing and word splitting
(shellcheck)
67-67: shellcheck reported issue in this script: SC2086:info:2:36: Double quote to prevent globbing and word splitting
(shellcheck)
91-91: shellcheck reported issue in this script: SC2086:info:2:44: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 YAMLlint (1.37.1)
.github/workflows/test-ui.yaml
[warning] 2-2: truthy value should be one of [false, true]
(truthy)
[error] 53-53: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/test-wasm.yaml
[warning] 2-2: truthy value should be one of [false, true]
(truthy)
[error] 45-45: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/npm-release.yaml
[warning] 2-2: truthy value should be one of [false, true]
(truthy)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 119-119: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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 (29)
packages/sqlite-web-core/Cargo.toml (1)
2-2: Core crate rename looks correct.
Matches the workspace rename and aligns with other changes.Cargo.toml (1)
3-5: No stale “sqlite-worker” references found
Ran a repository-wide grep forsqlite-workerandsqlite-worker-core—no matches returned, and all Cargo.toml workspace entries now list onlysqlite-webandsqlite-web-core. Everything looks clean..vscode/settings.json (1)
8-11: Workspace crates rename looks correct (sqlite-worker → sqlite-web).The updated rust-analyzer.linkedProjects entries point to the new sqlite-web-core and sqlite-web crates and still include lib/rain.math.float. This aligns with the repo-wide rename and should keep IDE features working.
svelte-test/vitest.config.js (1)
79-80: Dependency pre-bundling exclusion is consistent with WASM package usage.Excluding sqlite-web from optimizeDeps is correct to avoid Vite trying to pre-bundle the WASM consumer.
svelte-test/src/routes/sql/+page.svelte (1)
4-5: Import source migration aligns with repo rename.Switching to sqlite-web preserves the same public API surface (init, SQLiteWasmDatabase) and complies with the async/Promise-based guideline.
svelte-test/src/routes/+page.svelte (2)
4-5: Import migration is correct; async usage complies with guidelines.Continuing to await init() and db.query() matches the project’s async API requirement.
68-74: Use Parameterized Queries with SQLiteWasmDatabase.queryTo prevent SQL injection—and to model best practice even in demos—avoid direct string interpolation of user input. Instead:
• Location
- File: svelte-test/src/routes/sql/+page.svelte
- Around lines 68–74, in the
onMount/executeQueryfunctions.• Suggested Change
- await db.query(` - INSERT OR IGNORE INTO users (name, email) - VALUES ('${newUserName.trim()}', '${newUserEmail.trim()}') - `); + // Use a parameterized query if supported + await db.query( + 'INSERT OR IGNORE INTO users (name, email) VALUES (?, ?)', + [newUserName.trim(), newUserEmail.trim()] + );• Fallback Sanitization
If the.query()method only accepts a single SQL string, minimally escape single quotes instead of interpolating raw input:- VALUES ('${newUserName.trim()}', '${newUserEmail.trim()}') + VALUES ( + '${newUserName.trim().replaceAll("'", "''")}', + '${newUserEmail.trim().replaceAll("'", "''")}' + )• Action Required
Please verify whetherSQLiteWasmDatabase.query()supports a second parameter for bound values (array of?-placeholders) or exposes aprepare/bindAPI. If it does, switch all data-modifying calls accordingly; otherwise ensure all interpolated values are properly escaped.svelte-test/vite.config.ts (2)
10-10: Rename looks correct and consistent with repo-wide migration.Plugin name update to sqlite-web-serve aligns with package rename and avoids dual registration with the old name.
36-36: No direct imports ofsqlite-web-coredetected—no need to exclude it.I ran searches for static ESM imports, dynamic imports, and CommonJS requires of
sqlite-web-coreand found no references in the codebase. Since it isn’t imported (directly or transitively), you can leave youroptimizeDeps.excludeunchanged as:exclude: ['sqlite-web']Feel free to ignore the previous suggestion to add
'sqlite-web-core'to the exclusion list.Likely an incorrect or invalid review comment.
flake.nix (2)
18-23: WASM tests now target sqlite-web packages — good move.Switching to packages/sqlite-web-core and packages/sqlite-web in test-wasm keeps CI aligned with the rename.
82-82: Dev shell includes all tasks and wasm-pack — LGTM.Having packages.build-submodules available in the shell improves local DX and parity with CI.
.github/workflows/test-wasm.yaml (1)
5-6: Concurrency expression may trip linters; simplify while keeping behavior.To satisfy actionlint and keep main un-cancelled, set cancel-in-progress: true and vary group to shard by branch.
Apply this diff:
concurrency: - group: ${{ github.ref }}-wasm - cancel-in-progress: ${{ github.ref != 'refs/heads/main' }} + group: ${{ github.workflow }}-${{ github.ref }}-wasm + cancel-in-progress: trueThis cancels in-progress runs per ref; main stays unaffected because group keys differ per commit anyway.
If you must never cancel on main, split the workflow or add a separate non-cancelling group for main via a second workflow.
Likely an incorrect or invalid review comment.
svelte-test/tests/integration/opfs-persistence.test.ts (1)
9-9: Migration to sqlite-web confirmed – no stale references remainAll references to
sqlite-workerhave been removed and integration tests now consistently import the type fromsqlite-web:
- No matches for
sqlite-workerimports or mentions in the repo- Every test in
svelte-test/tests/integrationimportsSQLiteWasmDatabasefromsqlite-web(e.g. lines 9–10 in each file)svelte-test/tests/integration/error-handling.test.ts (1)
9-9: Type import source updated to sqlite-web — good.Matches the new package name and keeps runtime free of side effects.
svelte-test/README.md (1)
71-81: Sample code now imports from sqlite-web — LGTM.The init-and-use flow remains accurate for the WASM entry. No runtime issues expected.
svelte-test/tests/fixtures/test-helpers.ts (2)
1-1: Import change looks good.The migration from
sqlite-workertosqlite-webis consistent with the PR's overall objective.
187-190: Test table additions look good.The addition of
bigint_testandcategoriestables to the cleanup routine properly supports the new database function tests.packages/sqlite-web-core/src/database_functions/rain_math.rs (3)
1-1: Import change looks good.Using
use super::*;is appropriate for accessing parent module exports.
4-4: Good visibility change for function.Making
rain_math_processpublic withpubis correct for the new module structure where registration happens in mod.rs.
103-184: Test coverage looks comprehensive.The tests properly cover U256 parsing, hex conversion, Float integration, and edge cases.
packages/sqlite-web-core/src/database_functions/bigint_sum.rs (1)
13-24: Robust error handling in add_value.The function properly validates empty strings and provides detailed error messages for parsing failures.
packages/sqlite-web-core/src/database_functions/mod.rs (3)
1-7: LGTM! Clean module structure and imports.Good organization with appropriate imports for I256, U256, Float, and SQLite FFI types.
17-60: Well-structured function registration with proper error handling.The registration logic correctly sets up both scalar (RAIN_MATH_PROCESS) and aggregate (BIGINT_SUM) functions with appropriate callbacks.
63-91: Good test coverage for CString edge cases.Tests properly verify CString conversion behavior including null byte handling.
.github/workflows/npm-release.yaml (2)
73-78: Confirm commit/tag are consistent with the working directory changeAfter moving npm version into pkg/, staging pkg/package.json is correct. No change required; just calling out the dependency on the previous fix.
If the root package.json was inadvertently changed before, you may need to revert it.
49-52: Harden shell: add pipefail; quote $GITHUB_ENV to avoid word-splittingShellcheck SC2086 warns about unquoted $GITHUB_ENV. Also, enable -Eeuo pipefail for safer execution.
- run: | - OLD_HASH=$(npm view @rainlanguage/sqlite-web@latest dist.shasum 2>/dev/null || echo "none") - echo "OLD_HASH=$OLD_HASH" >> $GITHUB_ENV - echo "old hash: $OLD_HASH" + run: | + set -Eeuo pipefail + OLD_HASH="$(npm view @rainlanguage/sqlite-web@latest dist.shasum 2>/dev/null || echo "none")" + echo "OLD_HASH=$OLD_HASH" >> "$GITHUB_ENV" + echo "old hash: $OLD_HASH"⛔ Skipped due to learnings
Learnt from: findolor PR: rainlanguage/sqlite-web#2 File: .github/workflows/npm-release.yaml:54-58 Timestamp: 2025-08-21T12:24:09.173Z Learning: The rainlanguage organization prefers to maintain consistency in GitHub Actions workflows across repositories, even when shellcheck suggests improvements. They prioritize organizational standards over individual script hardening suggestions.Learnt from: findolor PR: rainlanguage/sqlite-web#2 File: .github/workflows/npm-release.yaml:88-98 Timestamp: 2025-08-22T06:58:43.195Z Learning: The findolor user considers shellcheck warnings about unquoted environment variables in GitHub Actions workflows acceptable when using proven workflow patterns from working projects, prioritizing consistency over addressing minor shell safety issues.scripts/local-bundle.sh (3)
94-101: Pipeline robustness: ensure sed/awk failures fail the scriptWith -o pipefail (added earlier), any sed/awk failure will now exit non-zero. No code changes needed here; just acknowledging improved robustness after the shell flags change.
20-23: Nice checks and helpful diagnostics
- Verifying presence of generated artifacts (wasm/js)
- Inlining base64 and logging sizes and glue line counts
These improve debuggability. Keep it up.
Also applies to: 27-28, 101-109
97-101: Verify generated wasm-pack JS exportsThe
pkgdirectory (and itssqlite_web_core.jsfile) is produced at build time by wasm-pack and isn’t committed in the repo, so we can’t confirm transformations in this sandbox. Please:
- Run your build step (e.g.
wasm-pack build …) to populatepkg/sqlite_web_core.js.- In that directory, list any remaining exports:
rg -nP '^\s*export\s+' -C1 pkg/sqlite_web_core.js- Confirm there are no
export const …or other named exports (e.g.export let,export { foo, bar }) that need to be rewritten for the worker scope.- If you do find additional exports, extend your
sedcommand accordingly. For example:- sed 's/^export function /function /; ... ; s/import\.meta\.url/self.location.href/g' pkg/sqlite_web_core.js + sed 's/^export function /function /; \ s/^export class /class /; \ s/^export const /const /; \ s/^export let /let /; \ s/^export { initSync };/self.initSync = initSync;/; \ s/^export default __wbg_init;/self.wasm_bindgen = __wbg_init;/; \ s/import\.meta\.url/self.location.href/g' pkg/sqlite_web_core.jsThis manual check ensures your worker truly receives all bindings from the generated glue code.
| cache: "npm" | ||
|
|
||
| - run: nix develop -c build-submodules | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Fix trailing whitespace and ensure newline at EOF
YAMLlint reports trailing spaces on Line 35 and missing final newline. Trim the trailing spaces and end the file with a newline.
-
+Also ensure there is a newline at the very end of the file.
Also applies to: 119-119
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 35-35: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
.github/workflows/npm-release.yaml around lines 35 and 119: remove any trailing
whitespace characters at the end of lines 35 and 119 and ensure the file ends
with a single newline character (add a final empty line/line break at EOF) so
there is no trailing space and the file terminates with a newline.
| - name: Get Old Hash | ||
| run: | | ||
| OLD_HASH=$(npm view @rainlanguage/sqlite-web@latest dist.shasum 2>/dev/null || echo "none") | ||
| echo "OLD_HASH=$OLD_HASH" >> $GITHUB_ENV | ||
| echo "old hash: $OLD_HASH" | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Optional: compare sha512 integrity instead of sha1 shasum
npm exposes dist.integrity (sha512). Using that provides a stronger integrity check and avoids reliance on local shasum formatting.
- OLD_HASH="$(npm view @rainlanguage/sqlite-web@latest dist.shasum 2>/dev/null || echo "none")"
+ OLD_HASH="$(npm view @rainlanguage/sqlite-web@latest dist.integrity 2>/dev/null || echo "none")"You’ll also need to compute a matching sha512 integrity string for NEW_HASH (e.g., using openssl base64 encoding of the sha512 digest in SRI format) if you adopt this.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 actionlint (1.7.7)
48-48: shellcheck reported issue in this script: SC2086:info:2:30: Double quote to prevent globbing and word splitting
(shellcheck)
🤖 Prompt for AI Agents
.github/workflows/npm-release.yaml around lines 47 to 52: the workflow currently
fetches the package's shasum (sha1) via npm view and compares it to a locally
computed OLD_HASH; replace this with npm view ... dist.integrity to use the
package's sha512 SRI integrity string, and compute NEW_HASH in matching SRI
format for the local artifact by computing the raw sha512 digest,
base64-encoding it and prefixing with "sha512-" so the two values can be
compared directly; update the environment assignment and any print/log to use
the integrity string instead of the sha1 shasum.
| it('should handle hexadecimal input', async () => { | ||
| await db.query(` | ||
| INSERT INTO bigint_test (amount) VALUES | ||
| ('0x10'), -- 16 in decimal | ||
| ('0xFF'), -- 255 in decimal | ||
| ('0x100') -- 256 in decimal | ||
| `); | ||
|
|
||
| const result = await db.query('SELECT BIGINT_SUM(amount) as total FROM bigint_test'); | ||
| const data = JSON.parse(result.value || '[]'); | ||
|
|
||
| // 16 + 255 + 256 = 527 | ||
| expect(data[0].total).toBe('527'); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Improve hexadecimal test expectations.
The test for hexadecimal input should verify whether the BIGINT_SUM function is intended to support hex format (0x prefix). The current implementation likely doesn't support hex strings based on the I256::from_str usage.
🏁 Script executed:
#!/bin/bash
# Check if I256::from_str supports hex format
rg -n "from_str.*0x" --type rust packages/sqlite-web-core/Length of output: 172
🏁 Script executed:
#!/bin/bash
# List all Rust files in the database_functions directory
echo "=== database_functions/*.rs ==="
ls -1 packages/sqlite-web-core/src/database_functions/*.rs
echo
# Locate the bigint_sum implementation
echo "=== Searching for bigint_sum occurrences ==="
rg -n "bigint_sum" -g '*.rs' packages/sqlite-web-core/src/database_functions
# If bigint_sum.rs exists, show its top 200 lines for context
if [[ -f packages/sqlite-web-core/src/database_functions/bigint_sum.rs ]]; then
echo
echo "=== Showing packages/sqlite-web-core/src/database_functions/bigint_sum.rs (first 200 lines) ==="
sed -n '1,200p' packages/sqlite-web-core/src/database_functions/bigint_sum.rs
fiLength of output: 9492
Clarify hex parsing support in BIGINT_SUM
The aggregate’s add_value currently calls I256::from_str(value_str), which only handles decimal input; any string with a 0x prefix will fail to parse. Decide on one of the following approaches:
-
Add hexadecimal support
Inpackages/sqlite-web-core/src/database_functions/bigint_sum.rs, replace the decimal‐only parser with a prefixed parser:fn add_value(&mut self, value_str: &str) -> Result<(), String> { if value_str.trim().is_empty() { return Err("Empty string is not a valid number".to_string()); } - let num = I256::from_str(value_str) - .map_err(|e| format!("Failed to parse number '{}': {}", value_str, e))?; + let num = I256::from_str_prefixed(value_str) + .map_err(|e| format!("Failed to parse number '{}': {}", value_str, e))?; self.total = self.total.saturating_add(num); Ok(()) }(You can also call
I256::from_str_hexif you only want to accept hexadecimal, or fall back to decimal with.or_else(|_| I256::from_str_radix(value_str, 10)).) (strawlab.org) -
Remove or adjust the hex test
Insvelte-test/tests/database-functions/bigint-sum.test.ts(lines 278–291), either expect a parse error when feeding"0x…"values or drop the hexadecimal case entirely if hex support is not intended.
Committable suggestion skipped: line range outside the PR's diff.
| // 123 - 456 + 789 = 456, but since we can't go negative in U256, | ||
| // it should be 123 + 789 - 456 = 456 if order allows, or saturate | ||
| // The actual result depends on processing order, but should be valid | ||
| expect(data[0].total).toMatch(/^\d+$/); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Verify saturating arithmetic behavior comment.
Line 306 mentions "saturate" but the actual logic around saturating arithmetic should be verified. According to the implementation, saturating_add is used which prevents overflow/underflow but doesn't saturate negative sums to 0.
- // The actual result depends on processing order, but should be valid
- expect(data[0].total).toMatch(/^\d+$/);
+ // With saturating arithmetic, the result should be 456
+ expect(data[0].total).toBe('456'); // 123 - 456 + 789 = 456📝 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.
| // 123 - 456 + 789 = 456, but since we can't go negative in U256, | |
| // it should be 123 + 789 - 456 = 456 if order allows, or saturate | |
| // The actual result depends on processing order, but should be valid | |
| expect(data[0].total).toMatch(/^\d+$/); | |
| }); | |
| // 123 - 456 + 789 = 456, but since we can't go negative in U256, | |
| // it should be 123 + 789 - 456 = 456 if order allows, or saturate | |
| // With saturating arithmetic, the result should be 456 | |
| expect(data[0].total).toBe('456'); // 123 - 456 + 789 = 456 | |
| }); |
🤖 Prompt for AI Agents
In svelte-test/tests/database-functions/bigint-sum.test.ts around lines 304 to
308, the inline comment incorrectly states "saturate" — update the comment to
accurately describe that the implementation uses saturating_add which prevents
overflow by clamping at the numeric maximum but does not force negative sums to
zero; keep the test expectation as a non-negative integer
(expect(data[0].total).toMatch(/^\d+$/)) or, if you want stronger verification,
replace the comment with a brief note that results depend on processing order
and that saturating_add clamps high values (not negative results), leaving the
test assertion unchanged or tighten it to assert numeric-string non-negativity.
| PerformanceTracker | ||
| } from '../fixtures/test-helpers.js'; | ||
| import type { SQLiteWasmDatabase } from 'sqlite-worker'; | ||
| import type { SQLiteWasmDatabase } from 'sqlite-web'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Import switch to sqlite-web is correct; add coverage for BIGINT_SUM.
The rename is good. Given this PR adds BIGINT_SUM with signed 256-bit support and string inputs, add integration tests to exercise negative values, large magnitudes, and string parsing.
Here are two minimal additions you can drop into this file (or a new bigint-aggregate.test.ts):
it('BIGINT_SUM should handle negative and string inputs', async () => {
await db.query(`
CREATE TABLE bigints (v TEXT);
`);
await db.query(`
INSERT INTO bigints (v) VALUES ('-2'), ('5'), ('-3'), ('0')
`);
const res = await db.query(`SELECT BIGINT_SUM(v) AS sum FROM bigints`);
const data = JSON.parse(res.value || '[]');
expect(data[0].sum).toBe('0'); // expect string result to avoid JS number limits
});
it('BIGINT_SUM should handle values beyond JS safe integer', async () => {
await db.query(`DELETE FROM bigints`);
await db.query(`
INSERT INTO bigints (v) VALUES ('9007199254740993'), ('1')
`);
const res = await db.query(`SELECT BIGINT_SUM(v) AS sum FROM bigints`);
const data = JSON.parse(res.value || '[]');
expect(data[0].sum).toBe('9007199254740994');
});If saturation is exposed at SQL level, we can add edge tests with documented I256 max/min constants—happy to wire those once confirmed.
Please confirm the SQL-visible return type of BIGINT_SUM (TEXT vs BLOB/NUMERIC) so we assert against the correct type.
🤖 Prompt for AI Agents
In svelte-test/tests/integration/database-basic.test.ts around line 10, add
integration tests for the new BIGINT_SUM aggregate to cover negative values,
string inputs, and values beyond JS safe integer range; specifically create a
temporary table of TEXT values, insert negative and string-form numbers and very
large string numbers, run SELECT BIGINT_SUM(v) AS sum, parse the JSON response
and assert the returned sum equals the expected decimal string (treat the
SQL-visible return type as TEXT so comparisons are string-based); ensure you
also clear/reset the table between cases to isolate tests.
| server.middlewares.use('/pkg', (req, res, next) => { | ||
| const filePath = req.url?.substring(1); // remove leading slash | ||
| const fullPath = path.join(process.cwd(), 'node_modules/sqlite-worker', filePath || ''); | ||
| const fullPath = path.join(process.cwd(), 'node_modules/sqlite-web', filePath || ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix path traversal in dev server middleware.
req.url is not normalized/sanitized; an attacker can request ../../ to read arbitrary files under process.cwd(). Guard the join with a normalized baseDir check and decode the URL.
Apply this diff:
- const fullPath = path.join(process.cwd(), 'node_modules/sqlite-web', filePath || '');
+ const baseDir = path.resolve(process.cwd(), 'node_modules/sqlite-web');
+ const rel = path.normalize(decodeURIComponent(filePath || '')).replace(/^(\.\.[/\\])+/, '');
+ const fullPath = path.join(baseDir, rel);
+ if (!fullPath.startsWith(baseDir + path.sep)) {
+ res.statusCode = 403;
+ res.end('Forbidden');
+ return;
+ }Optionally set basic caching and handle a few more content types if needed:
if (fs.existsSync(fullPath)) {
+ res.setHeader('Cache-Control', 'no-cache');
if (fullPath.endsWith('.wasm')) {
res.setHeader('Content-Type', 'application/wasm');
} else if (fullPath.endsWith('.js')) {
- res.setHeader('Content-Type', 'application/javascript');
+ res.setHeader('Content-Type', 'application/javascript');
+ } else if (fullPath.endsWith('.json')) {
+ res.setHeader('Content-Type', 'application/json');
}
fs.createReadStream(fullPath).pipe(res);📝 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 fullPath = path.join(process.cwd(), 'node_modules/sqlite-web', filePath || ''); | |
| // guard against path traversal by resolving to a fixed base directory | |
| const baseDir = path.resolve(process.cwd(), 'node_modules/sqlite-web'); | |
| const rel = path | |
| .normalize(decodeURIComponent(filePath || '')) | |
| .replace(/^(\.\.[/\\])+/, ''); | |
| const fullPath = path.join(baseDir, rel); | |
| if (!fullPath.startsWith(baseDir + path.sep)) { | |
| res.statusCode = 403; | |
| res.end('Forbidden'); | |
| return; | |
| } | |
| if (fs.existsSync(fullPath)) { | |
| res.setHeader('Cache-Control', 'no-cache'); | |
| if (fullPath.endsWith('.wasm')) { | |
| res.setHeader('Content-Type', 'application/wasm'); | |
| } else if (fullPath.endsWith('.js')) { | |
| res.setHeader('Content-Type', 'application/javascript'); | |
| } else if (fullPath.endsWith('.json')) { | |
| res.setHeader('Content-Type', 'application/json'); | |
| } | |
| fs.createReadStream(fullPath).pipe(res); | |
| } else { | |
| next(); | |
| } |
| name: 'sqlite-web-serve', | ||
| configureServer(server) { | ||
| server.middlewares.use('/pkg', (req, res, next) => { | ||
| const filePath = req.url?.substring(1); | ||
| const fullPath = path.join(process.cwd(), 'node_modules/sqlite-worker', filePath || ''); | ||
| const fullPath = path.join(process.cwd(), 'node_modules/sqlite-web', filePath || ''); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix path resolution under the ‘/pkg’ mount (Express/Connect strips the mount prefix).
When using server.middlewares.use('/pkg', …), req.url is relative to the '/pkg' mount (e.g., '/sqlite_web_core_bg.wasm'), so joining node_modules/sqlite-web directly misses the 'pkg' segment. This will 404 for WASM/JS artifacts.
Apply:
-const fullPath = path.join(process.cwd(), 'node_modules/sqlite-web', filePath || '');
+const baseDir = path.join(process.cwd(), 'node_modules', 'sqlite-web', 'pkg');
+const fullPath = path.join(baseDir, filePath || '');📝 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.
| name: 'sqlite-web-serve', | |
| configureServer(server) { | |
| server.middlewares.use('/pkg', (req, res, next) => { | |
| const filePath = req.url?.substring(1); | |
| const fullPath = path.join(process.cwd(), 'node_modules/sqlite-worker', filePath || ''); | |
| const fullPath = path.join(process.cwd(), 'node_modules/sqlite-web', filePath || ''); | |
| name: 'sqlite-web-serve', | |
| configureServer(server) { | |
| server.middlewares.use('/pkg', (req, res, next) => { | |
| const filePath = req.url?.substring(1); | |
| const baseDir = path.join(process.cwd(), 'node_modules', 'sqlite-web', 'pkg'); | |
| const fullPath = path.join(baseDir, filePath || ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review continued from previous batch...
| echo "🧹 Clearing embedded_worker.js..." | ||
| echo "" > packages/sqlite-worker/src/embedded_worker.js | ||
| echo "" > packages/sqlite-web/src/embedded_worker.js | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Avoid prematurely truncating embedded_worker.js
Truncating before template generation risks leaving the repo in a broken state if the script exits early. Generate into place later (you already overwrite the file at Line 32).
-echo "" > packages/sqlite-web/src/embedded_worker.js
+: # do not pre-clear embedded_worker.js; it will be overwritten by the template generation below📝 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.
| echo "🧹 Clearing embedded_worker.js..." | |
| echo "" > packages/sqlite-worker/src/embedded_worker.js | |
| echo "" > packages/sqlite-web/src/embedded_worker.js | |
| echo "🧹 Clearing embedded_worker.js..." | |
| # do not pre-clear embedded_worker.js; it will be overwritten by the template generation below |
🤖 Prompt for AI Agents
In scripts/local-bundle.sh around lines 11 to 13, the script currently truncates
packages/sqlite-web/src/embedded_worker.js early which can leave the repo in a
broken state if the script exits; remove the early echo/truncate lines and
instead either generate the file into a temporary path or rely on the existing
overwrite at line 32 so the target file is only replaced when generation
completes; ensure no other parts of the script assume the file was truncated
earlier.
|
Closing this as I am going to open a new one without the chained PR changes |
Caution
Chained PR. Do not merge before #2
Motivation
The existing SQLite implementation lacked support for summing large integers that could include negative values. The previous approach using U256 (unsigned 256-bit integers) couldn't properly handle negative numbers in aggregate calculations.
Solution
Added a new BIGINT_SUM aggregate function that uses I256 (signed 256-bit integers) to properly handle both positive and negative large integer values. Key changes include:
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Tests
Documentation
Chores
Style