Skip to content

Conversation

@rhashimoto
Copy link
Collaborator

@rhashimoto rhashimoto commented Sep 24, 2025

Summary by CodeRabbit

  • New Features
    • Support binding boolean values in SQL statements (true/false mapped to 1/0).
    • Expose additional runtime methods to consumers.
  • Bug Fixes
    • Corrected handle-request state handling in OPFS cooperative sync.
    • Improved in-memory write reliability for Memory VFS.
  • Performance
    • Reused a shared TextEncoder for reduced allocations.
    • More robust memory views for WebAssembly-backed buffers.
  • Chores
    • Updated bundled SQLite to 3.50.1.
    • Added a GitHub issue template restricting issues to bug reports.
  • Tests
    • Added coverage for boolean parameter binding.

simolus3 and others added 23 commits April 30, 2025 17:24
Fix resetting `isHandleRequested` in `OPFSCoopSyncVFS`
…shimoto#272)

* Permit boolean values to be bound to statements, as 0/1

* Add test for boolean binding
…stance

Use a single `TextEncoder` instance
Export HEAP* module members for recent EMSDK changes.
Bumps [tar-fs](https://github.com/mafintosh/tar-fs) from 3.0.8 to 3.0.9.
- [Commits](mafintosh/tar-fs@v3.0.8...v3.0.9)

---
updated-dependencies:
- dependency-name: tar-fs
  dependency-version: 3.0.9
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
…yarn/tar-fs-3.0.9

Bump tar-fs from 3.0.8 to 3.0.9
* Replace Proxy with handwritten proxy for jRead/jWrite buffers.

* Replace Proxy with handwritten proxy for VFS return data.

---------

Co-authored-by: Roy Hashimoto <[email protected]>
* Use non-CAPTCHA SQLite download URL.

* Use consistent Makefile variable bracing.

---------

Co-authored-by: Roy Hashimoto <[email protected]>
* Fix WebLocksMixin state initialization.

* Don't fetch state in WebLocksMixin file control unnecessarily.

* Minor fixes.

---------

Co-authored-by: Roy Hashimoto <[email protected]>
@coderabbitai
Copy link

coderabbitai bot commented Sep 24, 2025

Walkthrough

This change updates dependencies and build metadata, adjusts demo import paths, adds runtime heap exports, replaces internal memory/view wrappers in FacadeVFS, refactors lock-state retrieval in WebLocksMixin, tweaks VFS behaviors, adds boolean binding in sqlite-api with tests, and updates global heap typings.

Changes

Cohort / File(s) Summary of changes
Repo config & build
.github/ISSUE_TEMPLATE/-do-not-post-anything-other-than-a-bug-report.md, Makefile, package.json
Added bug-report-only GitHub issue template. Updated SQLite to version-3.50.1 and tarball URL format. Bumped package version to 1.0.9.
Runtime exports & globals
src/extra_exported_runtime_methods.json, src/types/globals.d.ts
Exported HEAP32 and HEAPU8 runtime methods. Replaced global HEAPU32: Uint32Array with HEAP32: Int32Array.
Demo import paths
demo/hello/hello.js
Adjusted module import paths up one directory level; selected async build as active import; no control-flow changes.
VFS internals
src/FacadeVFS.js
Replaced Proxy-based DataView/HEAPU8 wrappers with DataViewProxy and Uint8ArrayProxy that rebind on memory resize; internal APIs now return these proxies.
Locks mixin
src/WebLocksMixin.js
Introduced #getLockState(fileId) to centralize lock-state creation/access; updated jLock/jUnlock/jCheckReservedLock/jFileControl to use it.
Example VFS tweaks
src/examples/MemoryVFS.js, src/examples/OPFSCoopSyncVFS.js
MemoryVFS: copy source now uses pData.subarray(). OPFSCoopSyncVFS: on unlock, clears file.persistentFile.isHandleRequested instead of this.isHandleRequested.
SQLite API & tests
src/sqlite-api.js, test/api_statements.js
Added boolean binding (true→1, false→0). Reused a shared TextEncoder. Added tests asserting boolean binds return integer values.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant App as Application
  participant API as sqlite-api.js
  participant C as SQLite C/WASM

  rect rgb(245,248,255)
  note over App,API: Prepare & bind (new: boolean support)
  App->>API: sqlite3.prepare(sql)
  App->>API: sqlite3.bind(param, true/false)
  alt value is boolean (new)
    API->>C: bind_int(param, 1 or 0)
    C-->>API: SQLITE_OK
  else other types
    API->>C: bind_* accordingly
    C-->>API: SQLITE_OK/ERR
  end
  end

  App->>API: sqlite3.step()
  API->>C: sqlite3_step
  C-->>API: row/done
  API-->>App: result
Loading
sequenceDiagram
  autonumber
  participant VFS as FacadeVFS
  participant Mem as WASM Memory
  participant View as DataViewProxy/Uint8ArrayProxy

  note over VFS,View: Accessing memory-backed data (changed: rebind-on-resize)
  VFS->>View: get view at offset/length
  View->>Mem: ensure buffer is current (rebind if detached)
  Mem-->>View: current buffer
  View-->>VFS: read/write via getters/setters
  alt Memory grows
    Mem-->>View: old buffer detached
    VFS->>View: subsequent access
    View->>Mem: reacquire new buffer
    Mem-->>View: new buffer
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning No pull request description was provided and the author has not used the required repository template, which includes mandatory license grant and certification checklist sections. As a result, the description fails to meet the repository’s structural requirements. Without any content or template usage, it is impossible to verify legal or contextual information for this change. Please add a pull request description using the provided template, ensuring that you complete the license grant and certification checklist sections to comply with the repository requirements.
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title clearly summarizes merging upstream wa-sqlite version 1.0.9, which matches the main change in this updateset. It succinctly conveys the update’s intent without extraneous detail. It is concise and specific enough for teammates to understand the PR’s purpose at a glance.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch rhashimoto-merge-upstream

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (9)
.github/ISSUE_TEMPLATE/-do-not-post-anything-other-than-a-bug-report.md (4)

2-2: Remove leading space in template name.

The leading space before "Do" will render oddly in the template picker.

-name: " Do not post anything other than a bug report"
+name: "Do not post anything other than a bug report"

4-6: Drop empty front‑matter keys instead of setting empty strings.

Empty strings for title/labels/assignees can behave unexpectedly; omitting them is cleaner.

-title: ''
-labels: ''
-assignees: ''

8-10: Add a minimal body to guide reporters.

Without content, users won’t see any instructions. Add a short section with required info.

 ---
 
-
+Please use this template only for reproducible bugs in the project code.
+
+Provide:
+- Version (commit or release)
+- Environment (browser/runtime)
+- Steps to reproduce
+- Expected vs. actual behavior
+- Relevant logs or errors

1-1: Optional: Consider renaming file to avoid leading hyphen.

The leading “-” only affects sort order; if unintentional, rename to do-not-post-anything-other-than-a-bug-report.md.

src/FacadeVFS.js (3)

416-418: Fix JSDoc return type to match implementation (DataViewProxy).
Prevents confusion and aids tooling.

-   * @returns {DataView}
+   * @returns {DataViewProxy}

429-432: Fix JSDoc return type or clarify contract.
This returns a Uint8Array-like proxy, not a real Uint8Array. Clarify to reduce misuse.

-   * @returns {Uint8Array}
+   * @returns {Uint8Array} (Uint8Array-like proxy; use .subarray() for a real Uint8Array)

486-613: Match TypedArray mutator return values for chainability.
copyWithin/fill/reverse/sort should return the array to mirror Uint8Array semantics and avoid surprising callers.

   copyWithin(target, start, end) {
-    this.#array.copyWithin(target, start, end);
+    return this.#array.copyWithin(target, start, end);
   }
@@
   fill(value, start, end) {
-    this.#array.fill(value, start, end);
+    return this.#array.fill(value, start, end);
   }
@@
   reverse() {
-    this.#array.reverse();
+    return this.#array.reverse();
   }
@@
   sort(compareFn) {
-    this.#array.sort(compareFn);
+    return this.#array.sort(compareFn);
   }
src/sqlite-api.js (1)

521-546: Add parallel boolean support in sqlite3.result for consistency.
Results from custom functions should mirror bind semantics.

   sqlite3.result = function(context, value) {
     switch (typeof value) {
       case 'number':
@@
       case 'string':
         sqlite3.result_text(context, value);
         break;
+      case 'boolean':
+        sqlite3.result_int(context, value ? 1 : 0);
+        break;
       default:
test/api_statements.js (1)

241-260: Good coverage: boolean bind as integer.
Consider adding a false case to assert 0 as well.

     it('should bind boolean', async function() {
@@
-      const storeValue = true;
-      const expectedRetrievedValue = 1;
+      const storeValue = true;
+      const expectedRetrievedValue = 1;
@@
     });
+
+    it('should bind boolean false', async function() {
+      let rc;
+      const sql = 'SELECT ?';
+      for await (const stmt of i(sqlite3.statements(db, sql))) {
+        rc = await sqlite3.bind$(stmt, 1, false);
+        expect(rc).toEqual(SQLite.SQLITE_OK);
+        while ((rc = await sqlite3.step(stmt)) !== SQLite.SQLITE_DONE) {
+          expect(rc).toEqual(SQLite.SQLITE_ROW);
+          expect(await sqlite3.column_type(stmt, 0)).toEqual(SQLite.SQLITE_INTEGER);
+          expect(await sqlite3.column_int(stmt, 0)).toEqual(0);
+        }
+      }
+    });
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1ffca81 and ba784f5.

⛔ Files ignored due to path filters (4)
  • dist/wa-sqlite-async.wasm is excluded by !**/dist/**, !**/*.wasm
  • dist/wa-sqlite-jspi.wasm is excluded by !**/dist/**, !**/*.wasm
  • dist/wa-sqlite.wasm is excluded by !**/dist/**, !**/*.wasm
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (12)
  • .github/ISSUE_TEMPLATE/-do-not-post-anything-other-than-a-bug-report.md (1 hunks)
  • Makefile (1 hunks)
  • demo/hello/hello.js (2 hunks)
  • package.json (1 hunks)
  • src/FacadeVFS.js (2 hunks)
  • src/WebLocksMixin.js (4 hunks)
  • src/examples/MemoryVFS.js (1 hunks)
  • src/examples/OPFSCoopSyncVFS.js (1 hunks)
  • src/extra_exported_runtime_methods.json (1 hunks)
  • src/sqlite-api.js (3 hunks)
  • src/types/globals.d.ts (1 hunks)
  • test/api_statements.js (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/examples/MemoryVFS.js (2)
test/vfs_xRead.js (2)
  • iOffset (26-26)
  • pData (25-25)
test/vfs_xWrite.js (2)
  • iOffset (26-26)
  • pData (25-25)
test/api_statements.js (1)
src/sqlite-api.js (7)
  • rc (492-492)
  • rc (700-708)
  • rc (735-735)
  • i (101-101)
  • i (363-363)
  • i (609-609)
  • sqlite3 (28-28)
src/FacadeVFS.js (1)
src/examples/OPFSPermutedVFS.js (1)
  • array (1204-1207)
⏰ 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)
  • GitHub Check: build (20.x)
🔇 Additional comments (12)
.github/ISSUE_TEMPLATE/-do-not-post-anything-other-than-a-bug-report.md (1)

1-11: No stray “11” at EOF
Verified absence of a standalone “11” in the file.

src/examples/OPFSCoopSyncVFS.js (1)

437-441: Good alignment of request state with the shared persistent file.

Clearing isHandleRequested on the shared PersistentFile object keeps the state consistent across connections and matches where the flag is now set. Looks great.

src/WebLocksMixin.js (1)

128-141: Nice encapsulation of lock-state initialization.

Centralizing the lazy creation logic in #getLockState() removes repetition and keeps the downstream call sites cleaner while preserving the existing defaults. 👍

demo/hello/hello.js (2)

7-9: Approve import path bump. All referenced files in dist/ (wa-sqlite.mjs, wa-sqlite-async.mjs, wa-sqlite-jspi.mjs) are present.


22-26: Approve VFS example import paths — All referenced files exist under src/examples.

package.json (1)

3-3: Version bump to 1.0.9 looks good.
Matches the upstream sync scope; no concerns.

src/examples/MemoryVFS.js (1)

119-119: Good: ensure a real Uint8Array source in set().
Using pData.subarray() avoids proxy pitfalls and keeps copying semantics predictable during WASM memory resizes.

src/extra_exported_runtime_methods.json (1)

13-15: Exporting HEAP32/HEAPU8 is appropriate.
Aligns with updated typings and runtime access patterns.

src/sqlite-api.js (2)

37-37: Good: shared TextEncoder reuse.
Reduces allocations on hot paths; consistent use across helpers looks good.

Also applies to: 42-42, 665-665


122-124: Nice: boolean binding support.
Mapping booleans to integers aligns with SQLite typing.

src/types/globals.d.ts (1)

14-14: HEAP32 addition is fine; HEAPU32 is still referenced in generated bundles — verify consumers.
rg found HEAPU32 only in generated files: dist/wa-sqlite.mjs, dist/wa-sqlite-async.mjs, dist/wa-sqlite-jspi.mjs — if these are build artifacts, either regenerate builds to match the types change or ensure downstream code expecting HEAPU32 remains compatible.

Makefile (1)

2-3: SQLite tarball URL and contents verified
HTTP 200 response; configure found at sqlite/configure and sqlite/autoconf/tea/configure.

@rhashimoto rhashimoto merged commit 643d969 into master Sep 24, 2025
2 checks passed
@rhashimoto rhashimoto deleted the rhashimoto-merge-upstream branch September 24, 2025 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants