refactor: replace pglite for a vault data - #263
Merged
Conversation
I've open a PR to patch types DefinitelyTyped/DefinitelyTyped#74764
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces PGLite with a sql.js-backed SQLite stack (main-thread + worker), adds FlexSearch worker-backed indexing/storage, migrates controllers/queries to SQLite, introduces SQLite migrations and utilities, removes lexeme system and PGLite artifacts, updates build (WASM/ESM) and test worker setup. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App (main thread)
participant ManagedDB as ManagedDatabase<SQLiteDB>
participant WorkerComlink as Comlink Host/Worker Proxy
participant SQLiteWorker as SQLite Worker (sql.js)
participant WASM as sql.js WASM
App->>ManagedDB: openSQLite(fileController)
ManagedDB->>WorkerComlink: spawn/initialize worker with data
WorkerComlink->>SQLiteWorker: init(data)
SQLiteWorker->>WASM: load sql.js/wasm
WASM-->>SQLiteWorker: ready
SQLiteWorker-->>WorkerComlink: ready
WorkerComlink-->>ManagedDB: database proxy
ManagedDB-->>App: migrations run, ManagedDatabase ready
App->>ManagedDB: query/exec SQL
ManagedDB->>WorkerComlink: proxy query(...)
WorkerComlink->>SQLiteWorker: exec -> WASM
WASM-->>SQLiteWorker: results
SQLiteWorker-->>WorkerComlink: results
WorkerComlink-->>ManagedDB: parsed rows
ManagedDB-->>App: rows
sequenceDiagram
participant NotesApp as Notes UI
participant NotesController as NotesController
participant FlexIndex as FlexSearchIndex (host)
participant IndexWorker as FlexSearch Index Worker
participant FSStorage as IFilesStorage (ComlinkHostFS)
NotesApp->>NotesController: search("term")
NotesController->>FlexIndex: query("term")
FlexIndex->>IndexWorker: search("term")
IndexWorker->>FSStorage: read shards (via ComlinkHostFS)
IndexWorker->>FlexIndex: return ids
FlexIndex-->>NotesController: candidate ids
NotesController->>ManagedDB: query DB filters -> ids
NotesController->>NotesController: intersect & paginate
NotesController-->>NotesApp: final notes list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/features/tags/controller/selectTagsWithResolvedNames.sql (1)
19-41:⚠️ Potential issue | 🟠 MajorUse
ORDER BYwithin thegroup_concat()function for guaranteed ordering.SQLite's
group_concat()does not reliably order results based on a pre-sorted CTE alone. While the workaround of ordering rows in the source CTE may work in some SQLite versions, the safest approach is to includeORDER BYdirectly in the aggregate function.Replace:
group_concat(name, '/') AS resolved_name FROM sorted_tagtree GROUP BY tag_tree_idWith:
group_concat(name, '/' ORDER BY segment_id DESC) AS resolved_name FROM sorted_tagtree GROUP BY tag_tree_idThis ensures the concatenation respects the
segment_id DESCordering across all SQLite versions that support it (3.30.0+). If supporting older SQLite versions, document the version requirement and test with edge cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/features/tags/controller/selectTagsWithResolvedNames.sql` around lines 19 - 41, The GROUP_CONCAT call in the subquery for resolved_name relies on the CTE ordering and may produce incorrect order in some SQLite versions; update the aggregate to include an explicit ORDER BY so GROUP_CONCAT(name, '/' ORDER BY segment_id DESC) is used (referencing the sorted_tagtree CTE, tag_tree_id grouping, and segment_id ordering) to guarantee the concatenation order for resolved_name in the query that joins tags t with subquery x.
🧹 Nitpick comments (6)
src/core/features/workspaces/WorkspacesController.test.ts (1)
5-5: Consider usingafterEachhook for test isolation.Unlike
TagsController.test.tswhich usescloseHook: afterEachandclearFS: true, this test uses the defaultafterAllhook. This means both tests share the same database state, which could lead to test interdependence or flaky tests if execution order changes.♻️ Suggested change for better test isolation
-const { getDB } = makeAutoClosedSQLiteDB(); +const { getDB } = makeAutoClosedSQLiteDB({ closeHook: afterEach, clearFS: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/features/workspaces/WorkspacesController.test.ts` at line 5, The test uses makeAutoClosedSQLiteDB() with the default afterAll closure which shares DB state across tests; update the call in WorkspacesController.test.ts (the makeAutoClosedSQLiteDB/getDB setup) to use the same isolation options as TagsController.test.ts by passing closeHook: afterEach and clearFS: true so the database and filesystem are reset after each test, ensuring test isolation.src/core/storage/database/sqlite/utils/wrapDB.ts (1)
13-37: Remove commented debug code.Line 22 contains a commented-out debug statement that should be removed before merging.
🧹 Proposed cleanup
try { const result = await db.query(sql, bindings); - // console.log("DBG", result.rows); if (scheme) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/storage/database/sqlite/utils/wrapDB.ts` around lines 13 - 37, In wrapSQLite (the exported function), remove the commented-out debug statement "// console.log("DBG", result.rows);" inside the Proxy getter so no commented debug code remains; search within the get(...) implementation of wrapSQLite for any other leftover console.log debug comments and delete them as well to keep the production code clean.src/core/features/workspaces/WorkspacesController.ts (2)
60-67: Schema validation on DELETE is misleading.The
DELETEstatement doesn't include aRETURNINGclause, so it returns no rows. PassingworkspaceTypeas the schema suggests rows are expected. Either addRETURNING *to get deleted rows, or remove the schema parameter.💡 Option A: Remove schema (if deleted rows aren't needed)
public async delete(ids: string[]) { const db = wrapSQLite(this.db.get()); - await db.query( - qb.sql`DELETE FROM workspaces WHERE id IN (${qb.values(ids)})`, - workspaceType, - ); + await db.query( + qb.sql`DELETE FROM workspaces WHERE id IN (${qb.values(ids)})`, + ); }💡 Option B: Add RETURNING (if deleted rows are needed)
- await db.query( - qb.sql`DELETE FROM workspaces WHERE id IN (${qb.values(ids)})`, - workspaceType, - ); + return db.query( + qb.sql`DELETE FROM workspaces WHERE id IN (${qb.values(ids)}) RETURNING *`, + workspaceType, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/features/workspaces/WorkspacesController.ts` around lines 60 - 67, The delete method in WorkspacesController currently calls db.query with a DELETE that returns no rows but still passes workspaceType as the schema; either remove the schema parameter or make the SQL return deleted rows. Fix by updating the db.query call inside delete(ids: string[]) — Option A: remove the second arg (workspaceType) if you don't need deleted rows; Option B: append RETURNING * to the SQL (qb.sql`DELETE FROM workspaces WHERE id IN (${qb.values(ids)}) RETURNING *`) if you need the deleted workspace records and keep workspaceType.
32-35: UnnecessaryORDER BY rowidfor single-row lookup.When selecting by primary key (
id),ORDER BY rowidis unnecessary since at most one row will be returned. This appears to be leftover from bulk query patterns.🧹 Proposed simplification
const [info] = await db.query( - qb.sql`SELECT * FROM workspaces WHERE id=${id} ORDER BY rowid`, + qb.sql`SELECT * FROM workspaces WHERE id=${id}`, workspaceType, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/features/workspaces/WorkspacesController.ts` around lines 32 - 35, The SELECT by primary key in db.query uses an unnecessary ORDER BY rowid which is redundant for a single-row lookup; remove the ORDER BY rowid clause from the qb.sql call in WorkspacesController (the query that assigns const [info]) so the SQL becomes a simple SELECT * FROM workspaces WHERE id=${id} executed with workspaceType via db.query/qb.sql, leaving the rest of the logic (info variable, id parameter, and workspaceType) unchanged.src/core/storage/database/sqlite/openSQLite.ts (1)
13-22: Environment detection may be unreliable in bundled environments.The check
typeof process !== 'undefined'can be true in browser bundles where webpack or other bundlers polyfillprocess. Consider a more explicit approach or configuration flag.💡 Suggested alternative
- const isNode = typeof process !== 'undefined'; + const isNode = + typeof process !== 'undefined' && + process.versions != null && + process.versions.node != null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/storage/database/sqlite/openSQLite.ts` around lines 13 - 22, The environment detection using isNode = typeof process !== 'undefined' is brittle in bundled/browser polyfilled environments; update the detection in openSQLite.ts to use a reliable check (e.g., check for process?.versions?.node or absence of window/globalThis, or better: accept an explicit config flag) and branch accordingly when importing SQLiteDatabase vs SQLiteDatabaseWorker; modify the isNode assignment and subsequent conditional that imports SQLiteDatabase and SQLiteDatabaseWorker (references: isNode, SQLiteDatabase, SQLiteDatabaseWorker) so the code uses the explicit/node-version check or a passed-in option rather than typeof process !== 'undefined'.src/core/storage/database/sqlite/SQLiteDatabaseWorker.ts (1)
7-12: Consider adding explicit type annotation fordbproperty.The
protected dbfield lacks an explicit type annotation. While TypeScript can infer it, an explicit type improves readability.💡 Suggested improvement
export class SQLiteDatabaseWorker implements SQLiteDB { - protected db; + protected db: Promise<Remote<SQLiteDBWorker>>; constructor(data?: ArrayLike<number> | Buffer | null) {You'll need to import
Remotefromcomlink:import { proxy, wrap, Remote } from 'comlink';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/storage/database/sqlite/SQLiteDatabaseWorker.ts` around lines 7 - 12, Add an explicit type for the protected db field and import Remote from comlink: the db field is assigned this.db = db.init(data).then(() => db) so it should be typed as a Promise<Remote<SQLiteDBWorker>> (or Remote<SQLiteDBWorker> | Promise<Remote<SQLiteDBWorker>> if you prefer the union). Import Remote from 'comlink' alongside existing wrap/proxy usage and update the declaration in class SQLiteDatabaseWorker (reference symbols: SQLiteDatabaseWorker, db, SQLWorker, wrap, SQLiteDBWorker, Remote) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/storage/database/sqlite/openSQLite.ts`:
- Around line 38-41: getData currently returns data.buffer which can reference a
larger ArrayBuffer (e.g., WASM memory) and cause corruption; call db.export(),
make a copy of the Uint8Array (e.g., data.slice()) and return that copy's
ArrayBuffer so only the exported bytes are included. Update the getData
implementation (the async getData function that calls db.export()) to return
data.slice().buffer (or otherwise copy the bytes into a new ArrayBuffer) instead
of data.buffer.
In `@src/core/storage/database/sqlite/scheme.sql`:
- Around line 44-49: The attached_tags table lacks foreign key constraints for
the source and target columns; update the attached_tags definition to enforce
referential integrity by making source reference tags(id) and target reference
notes(id) (e.g., change the column definitions to include REFERENCES tags(id)
and REFERENCES notes(id)); ensure the existing workspace_id REFERENCES
workspaces(id) remains and consider whether you want ON DELETE/UPDATE behavior
(e.g., CASCADE or RESTRICT) for these new foreign keys if desired.
In `@src/core/storage/database/sqlite/SQLiteDatabase.ts`:
- Around line 39-42: The updateHook callback in SQLiteDatabase (db.updateHook)
currently invokes each registered onChange callback directly which allows
exceptions from onChange listeners to propagate and abort write queries; modify
the invocation inside SQLiteDatabase's updateHook (and the onChange callback
registration logic) to call each callback within a try-catch, swallowing or
logging the error (e.g., processLogger.warn or console.warn) but not rethrowing,
so a failing onChange listener cannot interrupt the originating write operation.
In `@src/core/storage/database/sqlite/SQLiteDatabase.worker.ts`:
- Around line 12-17: The close() implementation leaves the module-scoped db
reference pointing to a closed SQLiteDatabase, causing init() to try closing it
again; after successfully awaiting db.close() in the close() method, set the db
reference to null (or undefined) so init() can create a fresh SQLiteDatabase
instance without attempting to close an already-closed object—update the close()
function that currently references db (and the init() function that checks db)
to rely on db being null when no active DB exists.
In `@src/features/App/index.tsx`:
- Around line 19-34: Remove the development/debug block that unconditionally
calls openSQLite(new FileController('/db', new InMemoryFS())) and attaches
internals to window (references: openSQLite, FileController, InMemoryFS,
db.get(), window.db, window.time, window.dbInsert, db.get().onChange), because
it creates a module-level side effect, exposes the DB globally, and contains
invalid SQL and unconditional console logging; either delete the entire block or
move it behind a strict development-only guard (e.g., NODE_ENV ===
'development') and remove global assignments and console logs, replace the
invalid query `SELECT now() as now` with a proper SQLite expression (e.g.,
datetime('now')) if you keep any test code, and ensure any onChange handlers are
registered only in appropriate runtime code paths rather than at module import.
---
Outside diff comments:
In `@src/core/features/tags/controller/selectTagsWithResolvedNames.sql`:
- Around line 19-41: The GROUP_CONCAT call in the subquery for resolved_name
relies on the CTE ordering and may produce incorrect order in some SQLite
versions; update the aggregate to include an explicit ORDER BY so
GROUP_CONCAT(name, '/' ORDER BY segment_id DESC) is used (referencing the
sorted_tagtree CTE, tag_tree_id grouping, and segment_id ordering) to guarantee
the concatenation order for resolved_name in the query that joins tags t with
subquery x.
---
Nitpick comments:
In `@src/core/features/workspaces/WorkspacesController.test.ts`:
- Line 5: The test uses makeAutoClosedSQLiteDB() with the default afterAll
closure which shares DB state across tests; update the call in
WorkspacesController.test.ts (the makeAutoClosedSQLiteDB/getDB setup) to use the
same isolation options as TagsController.test.ts by passing closeHook: afterEach
and clearFS: true so the database and filesystem are reset after each test,
ensuring test isolation.
In `@src/core/features/workspaces/WorkspacesController.ts`:
- Around line 60-67: The delete method in WorkspacesController currently calls
db.query with a DELETE that returns no rows but still passes workspaceType as
the schema; either remove the schema parameter or make the SQL return deleted
rows. Fix by updating the db.query call inside delete(ids: string[]) — Option A:
remove the second arg (workspaceType) if you don't need deleted rows; Option B:
append RETURNING * to the SQL (qb.sql`DELETE FROM workspaces WHERE id IN
(${qb.values(ids)}) RETURNING *`) if you need the deleted workspace records and
keep workspaceType.
- Around line 32-35: The SELECT by primary key in db.query uses an unnecessary
ORDER BY rowid which is redundant for a single-row lookup; remove the ORDER BY
rowid clause from the qb.sql call in WorkspacesController (the query that
assigns const [info]) so the SQL becomes a simple SELECT * FROM workspaces WHERE
id=${id} executed with workspaceType via db.query/qb.sql, leaving the rest of
the logic (info variable, id parameter, and workspaceType) unchanged.
In `@src/core/storage/database/sqlite/openSQLite.ts`:
- Around line 13-22: The environment detection using isNode = typeof process !==
'undefined' is brittle in bundled/browser polyfilled environments; update the
detection in openSQLite.ts to use a reliable check (e.g., check for
process?.versions?.node or absence of window/globalThis, or better: accept an
explicit config flag) and branch accordingly when importing SQLiteDatabase vs
SQLiteDatabaseWorker; modify the isNode assignment and subsequent conditional
that imports SQLiteDatabase and SQLiteDatabaseWorker (references: isNode,
SQLiteDatabase, SQLiteDatabaseWorker) so the code uses the explicit/node-version
check or a passed-in option rather than typeof process !== 'undefined'.
In `@src/core/storage/database/sqlite/SQLiteDatabaseWorker.ts`:
- Around line 7-12: Add an explicit type for the protected db field and import
Remote from comlink: the db field is assigned this.db = db.init(data).then(() =>
db) so it should be typed as a Promise<Remote<SQLiteDBWorker>> (or
Remote<SQLiteDBWorker> | Promise<Remote<SQLiteDBWorker>> if you prefer the
union). Import Remote from 'comlink' alongside existing wrap/proxy usage and
update the declaration in class SQLiteDatabaseWorker (reference symbols:
SQLiteDatabaseWorker, db, SQLWorker, wrap, SQLiteDBWorker, Remote) accordingly.
In `@src/core/storage/database/sqlite/utils/wrapDB.ts`:
- Around line 13-37: In wrapSQLite (the exported function), remove the
commented-out debug statement "// console.log("DBG", result.rows);" inside the
Proxy getter so no commented debug code remains; search within the get(...)
implementation of wrapSQLite for any other leftover console.log debug comments
and delete them as well to keep the production code clean.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d510c6d-ee7a-45ed-aac8-d2e00ab5e237
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
package.jsonpatches/@types+sql.js+1.4.9.patchscripts/webpack/app.tsscripts/webpack/shared.tssrc/__tests__/utils/makeAutoClosedSQLiteDB.tssrc/core/features/tags/controller/TagsController.test.tssrc/core/features/tags/controller/TagsController.tssrc/core/features/tags/controller/selectTagsWithResolvedNames.sqlsrc/core/features/workspaces/WorkspacesController.test.tssrc/core/features/workspaces/WorkspacesController.tssrc/core/storage/database/sqlite/SQLiteDatabase.test.tssrc/core/storage/database/sqlite/SQLiteDatabase.tssrc/core/storage/database/sqlite/SQLiteDatabase.worker.tssrc/core/storage/database/sqlite/SQLiteDatabaseWorker.tssrc/core/storage/database/sqlite/index.tssrc/core/storage/database/sqlite/openSQLite.tssrc/core/storage/database/sqlite/scheme.sqlsrc/core/storage/database/sqlite/utils/query-builder.tssrc/core/storage/database/sqlite/utils/wrapDB.tssrc/features/App/index.tsxsrc/types.d.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/features/notes/controller/NotesController.ts (1)
121-165:⚠️ Potential issue | 🟠 MajorPostgreSQL-specific search code is incompatible with SQLite.
The search query construction (lines 121-165) uses PostgreSQL-specific features that SQLite doesn't support:
regexp_split_to_table(PostgreSQL regex function)plainto_tsquery,to_tsquery,tsquery_and_agg(full-text search)ts_rank_cd(ranking function)text_tsv @@ et.query(tsquery matching)%trigram similarity operatorThis code path will throw runtime errors if
searchis provided. This aligns with the skipped search tests, but the code should either be removed or guarded to prevent runtime failures.🔧 Suggested approach
Consider one of these options:
- Remove the search code entirely until SQLite FTS5 support is implemented
- Add a runtime guard that throws a clear error message:
// Search if (search) { + throw new Error('Full-text search is not yet implemented for SQLite backend'); // In case a user input contains typos...
- Implement SQLite FTS5-based search as a replacement
🧹 Nitpick comments (2)
src/core/features/notes/controller/NotesController.test.ts (1)
610-612: Track skipped search tests for future implementation.The
Notes searchsuite is skipped because it relies on PostgreSQL-specific features (pg_trgm,tsquery,tsvector,regexp_split_to_table) that SQLite doesn't support natively.Consider adding a TODO comment or tracking issue to implement SQLite-compatible full-text search (e.g., using SQLite's FTS5 extension) to restore this functionality.
Would you like me to open an issue to track implementing SQLite-compatible full-text search?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/features/notes/controller/NotesController.test.ts` around lines 610 - 612, The test suite 'Notes search' is skipped via describe.skip and relies on PostgreSQL-only features (pg_trgm, tsquery, tsvector, regexp_split_to_table) so we should track restoring it for SQLite; add a brief TODO comment above the describe.skip mentioning the need to implement SQLite-compatible full-text search (e.g., FTS5) or reference a tracking issue/issue ID, and optionally create that issue; locate the skipped suite using the 'describe.skip("Notes search")' block (and related symbols createFileControllerMock and openSQLite/dbPromise) and insert the TODO or link to the issue so future work restores the tests when SQLite FTS support is added.src/core/storage/database/sqlite/SQLiteDatabase.test.ts (1)
7-25: LGTM on custom functions tests, but consider timezone-sensitive assertion.The tests for
now(),timestamp(), andgen_random_uuid()are well-structured. However, the assertion on line 19-20 uses'10/10/2000'which parses differently across timezones:await expect( db.get().query(`SELECT timestamp('10/10/2000') as time`), ).resolves.toEqual([{ time: 971128800000 }]);The expected epoch
971128800000corresponds to a specific timezone. Consider using an ISO 8601 format like'2000-10-10T00:00:00.000Z'for deterministic behavior across environments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/storage/database/sqlite/SQLiteDatabase.test.ts` around lines 7 - 25, The timestamp test using SELECT timestamp('10/10/2000') in SQLiteDatabase.test.ts is timezone-sensitive; update the query to use an ISO 8601 UTC string (e.g., '2000-10-10T00:00:00.000Z') when calling db.get().query for the timestamp() check and adjust the expected epoch value (the assertion that checks timestamp(...) equals 971128800000) to the deterministic UTC epoch for that ISO string so the test for timestamp() is consistent across timezones; keep the other assertions for now(), timestamp('now'), and gen_random_uuid() unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/features/notes/controller/NotesController.ts`:
- Around line 256-260: The code in NotesController.ts uses ES2025 iterator
helpers on the "ids" iterator (the expression assigned to "sortedNotes") which
isn't guaranteed to be transpiled for your es2022 webpack target; either change
the build target in your webpack config to 'es2025' or refactor the
"sortedNotes" creation to avoid iterator helpers by converting "ids" to a plain
array first (e.g., use Array.from(ids) or [...ids]) and then call .map(id =>
rowsMap.get(id)) and .filter(note => note !== undefined) to produce the final
array, keeping references to the "ids" iterator, "rowsMap", and the
"sortedNotes" variable intact.
---
Nitpick comments:
In `@src/core/features/notes/controller/NotesController.test.ts`:
- Around line 610-612: The test suite 'Notes search' is skipped via
describe.skip and relies on PostgreSQL-only features (pg_trgm, tsquery,
tsvector, regexp_split_to_table) so we should track restoring it for SQLite; add
a brief TODO comment above the describe.skip mentioning the need to implement
SQLite-compatible full-text search (e.g., FTS5) or reference a tracking
issue/issue ID, and optionally create that issue; locate the skipped suite using
the 'describe.skip("Notes search")' block (and related symbols
createFileControllerMock and openSQLite/dbPromise) and insert the TODO or link
to the issue so future work restores the tests when SQLite FTS support is added.
In `@src/core/storage/database/sqlite/SQLiteDatabase.test.ts`:
- Around line 7-25: The timestamp test using SELECT timestamp('10/10/2000') in
SQLiteDatabase.test.ts is timezone-sensitive; update the query to use an ISO
8601 UTC string (e.g., '2000-10-10T00:00:00.000Z') when calling db.get().query
for the timestamp() check and adjust the expected epoch value (the assertion
that checks timestamp(...) equals 971128800000) to the deterministic UTC epoch
for that ISO string so the test for timestamp() is consistent across timezones;
keep the other assertions for now(), timestamp('now'), and gen_random_uuid()
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17c74c55-0e24-4026-a88b-a8146297c844
📒 Files selected for processing (5)
src/core/features/notes/controller/NotesController.test.tssrc/core/features/notes/controller/NotesController.tssrc/core/storage/database/sqlite/SQLiteDatabase.test.tssrc/core/storage/database/sqlite/SQLiteDatabase.tssrc/core/storage/database/sqlite/scheme.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/storage/database/sqlite/scheme.sql
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
src/core/features/index/flexsearch/FlexSearchStorage.ts (2)
1155-1161: Comma operator pattern reduces readability.The
(map.get(k) ?? (map.set(k, []), map.get(k)!))pattern is clever but obscure. Consider a helper or explicit assignment for clarity.♻️ Proposed refactor for readability
+ // Helper for get-or-create pattern + const getOrCreate = <K, V>(map: Map<K, V[]>, key: K): V[] => { + let arr = map.get(key); + if (!arr) { + arr = []; + map.set(key, arr); + } + return arr; + }; const ssi = shardOf(id, this._shards.store); - (storeRm.get(ssi) ?? (storeRm.set(ssi, []), storeRm.get(ssi)!)).push(id); + getOrCreate(storeRm, ssi).push(id); const regi = shardOf(id, this._shards.reg); - (regRm.get(regi) ?? (regRm.set(regi, []), regRm.get(regi)!)).push(id); + getOrCreate(regRm, regi).push(id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/features/index/flexsearch/FlexSearchStorage.ts` around lines 1155 - 1161, The code uses a hard-to-read comma-operator pattern when ensuring arrays exist in maps: (map.get(k) ?? (map.set(k, []), map.get(k)!)). Replace this with a clear helper or explicit assignment: create or use a helper like getOrInit(map, key) or inline: let arr = map.get(key); if (!arr) { arr = []; map.set(key, arr); } arr.push(id). Apply this change where shardOf(id, this._shards.store) and shardOf(id, this._shards.reg) are used with storeRm and regRm to improve readability and maintain behavior.
715-863: Consider extracting commit phases into helper methods.The
commit()method is ~150 lines with 9 distinct phases. While well-commented, extracting each phase into a private method (e.g.,_commitMap(),_commitCtx(),_commitRefs()) would improve testability and readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/features/index/flexsearch/FlexSearchStorage.ts` around lines 715 - 863, The commit() method is large and contains 9 phases; extract each phase into small private helpers to improve readability and testability. Create methods like _commitDeletions(fs), _commitMap(fs, pendingRefs), _commitCtx(fs, pendingRefs), _commitRefs(pendingRefs), _commitStore(fs), _commitRegistry(fs), _commitTags(fs), _clearAccumulators(fs) and keep the final _flush() call; preserve the original behavior and ordering (including Promise.all shard preloads and awaits), pass/return pendingRefs where needed, and ensure you call the existing helpers used inside commit (e.g., _groupBy, _mapShard, _mapSync, _mergeBkts, _markDirty, _ctxShard, _ctxSync, _refShard, _refSync, _storeShard, _storeSync, _regShard, _regSync) so logic and concurrency remain identical.src/core/features/index/flexsearch/FlexSearchStorage.test.ts (1)
6-25: Tests are order-dependent due to shared state.The
indexFsinstance is shared at module scope, making "Reuse the index" depend on "Fill the index" running first. This works but creates fragile tests that will fail if run in isolation, in parallel, or in reverse order.Consider making the dependency explicit or restructuring:
♻️ Option 1: Use test lifecycle hooks
+let indexFs: InMemoryFS; + +beforeAll(async () => { + indexFs = new InMemoryFS(); + const index = new Index(); + await index.mount(new FlexSearchStorage(indexFs)); + await index.add('foo', 'The foo content'); + await index.add('bar', 'The bar content'); + await index.commit(); +}); -const indexFs = new InMemoryFS(); -test('Fill the index', async () => { - const index = new Index(); - await index.mount(new FlexSearchStorage(indexFs)); - - await expect(index.search('foo')).resolves.toEqual([]); - - await index.add('foo', 'The foo content'); - await index.add('bar', 'The bar content'); - await index.commit(); - await expect(index.search('foo')).resolves.toEqual(['foo']); -}); +test('Search returns expected document after commit', async () => { + const index = new Index(); + await index.mount(new FlexSearchStorage(indexFs)); + await expect(index.search('foo')).resolves.toEqual(['foo']); +}); test('Reuse the index', async () => {♻️ Option 2: Combine into a single test
-const indexFs = new InMemoryFS(); - -test('Fill the index', async () => { - const index = new Index(); - await index.mount(new FlexSearchStorage(indexFs)); - - await expect(index.search('foo')).resolves.toEqual([]); - - await index.add('foo', 'The foo content'); - await index.add('bar', 'The bar content'); - await index.commit(); - await expect(index.search('foo')).resolves.toEqual(['foo']); -}); - -test('Reuse the index', async () => { - const index = new Index(); - await index.mount(new FlexSearchStorage(indexFs)); - - await expect(index.search('foo')).resolves.toEqual(['foo']); -}); +test('Index persists data across mount cycles', async () => { + const indexFs = new InMemoryFS(); + + // Fill the index + const index1 = new Index(); + await index1.mount(new FlexSearchStorage(indexFs)); + await expect(index1.search('foo')).resolves.toEqual([]); + await index1.add('foo', 'The foo content'); + await index1.add('bar', 'The bar content'); + await index1.commit(); + await expect(index1.search('foo')).resolves.toEqual(['foo']); + + // Reuse the index with a fresh instance + const index2 = new Index(); + await index2.mount(new FlexSearchStorage(indexFs)); + await expect(index2.search('foo')).resolves.toEqual(['foo']); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/features/index/flexsearch/FlexSearchStorage.test.ts` around lines 6 - 25, Tests share a single InMemoryFS instance (indexFs) at module scope causing order-dependent failures; make each test use a fresh storage by creating a new InMemoryFS for each test (e.g., move indexFs = new InMemoryFS() into a beforeEach or instantiate it inside each test) and ensure Index.mount(new FlexSearchStorage(indexFs)) uses that fresh instance so "Fill the index" no longer affects "Reuse the index".src/core/features/notes/bin/DeletedNotesController.ts (1)
40-47: Prefer omittingupdatedAtfilter when not used.Passing
{ updatedAt: { to: undefined } }is ambiguous and depends on downstream query-builder handling. Build the filter object conditionally instead.♻️ Proposed refactor
- const noteIds = await notes.query({ - deletedAt: { - to: new Date(validityDate), - }, - updatedAt: { - to: considerModificationTime ? new Date(validityDate) : undefined, - }, - }); + const noteIds = await notes.query({ + deletedAt: { + to: new Date(validityDate), + }, + ...(considerModificationTime + ? { + updatedAt: { + to: new Date(validityDate), + }, + } + : {}), + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/features/notes/bin/DeletedNotesController.ts` around lines 40 - 47, The query building in DeletedNotesController passes updatedAt: { to: undefined } when considerModificationTime is false, which can be ambiguous for downstream query builders; change the code to construct the filter object conditionally so that updatedAt is only included when considerModificationTime is true (e.g., build a base filter with deletedAt and then add updatedAt: { to: new Date(validityDate) } only when considerModificationTime is true) before calling notes.query(...) so notes.query receives no updatedAt key when it shouldn’t be applied.src/core/features/notes/controller/__tests__/NotesController.bench.ts (1)
156-172: Benchmark measures indexing + search, not pure search performance.The search benchmark calls
indexScanner.update()inside each iteration (line 160), which re-indexes notes before every search. This measures combined indexing + search latency rather than isolated search performance.If the intent is to benchmark search alone, move the indexing to a setup phase:
♻️ Proposed fix to isolate search performance
describe('Note search', () => { bench( 'Search note with random text', async function () { - await indexScanner.update(); await notes.get({ // Text with typo search: { text: `powr` }, limit: 10, }); }, { ...benchConfig, iterations: 10, + async setup(_task, mode) { + if (mode !== 'warmup') return; + await indexScanner.update(); + }, }, ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/features/notes/controller/__tests__/NotesController.bench.ts` around lines 156 - 172, The benchmark currently calls indexScanner.update() inside the bench callback (in the bench labelled 'Search note with random text'), which causes each iteration to re-index and measures indexing+search; move the indexing step out of the per-iteration workload by running indexScanner.update() once in a setup phase (e.g., before the bench or in a beforeAll/benchmark setup) so that the bench callback only calls notes.get({ search: { text: 'powr' }, limit: 10 }) to measure pure search performance.src/core/features/notes/controller/NotesController.test.ts (1)
806-847: Tracked: commented-out test block for index actuality.The TODO comment and commented-out test for lexeme updates is acceptable for this transitional PR. Consider creating a tracking issue or removing the dead code once the new index-based workflow is fully validated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/features/notes/controller/NotesController.test.ts` around lines 806 - 847, The test file contains a large commented-out block testing lexeme index actuality (mentions NotesController, LexemesRegistry, lexemes.getList(), lexemes.index(), lexemes.prune(), registry.update(), registry.get()) which should not remain as dead code; either remove the commented test entirely or convert it into a tracked TODO by creating a short task/issue in your tracker and replace the commented block with a one-line TODO that references the new issue ID and a brief reason (e.g., "restore lexeme index actuality test — tracked in ISSUE-123"); ensure the TODO references NotesController/LexemesRegistry so it’s discoverable.src/core/features/notes/controller/NotesTextIndex.ts (1)
60-65: Type assertion on search results.
searchAsyncreturnsId[]whereIdcan benumber | stringdepending on how documents were indexed. The castas string[]is safe given thatNotesTextIndexScanneradds notes with string IDs, but this assumption isn't enforced at compile time.Consider adding a runtime check or documenting the contract that only string IDs are supported.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/features/notes/controller/NotesTextIndex.ts` around lines 60 - 65, The query method in NotesTextIndex.ts unsafely casts index.searchAsync(search) to string[]; instead enforce the string-ID contract at runtime or return a safe union: call getIndex() then await index.searchAsync(search) and validate each Id from searchAsync is a string (or map number->string) before returning, otherwise throw a descriptive error; alternatively change the method signature to return (string|number)[] if you want to accept numeric IDs. Reference: query, getIndex, searchAsync and the NotesTextIndexScanner that inserts string IDs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/features/index/flexsearch/FlexSearchStorage.ts`:
- Around line 580-603: The _flush method currently clears entry.dirty before the
async write completes which can cause data loss; modify _flush so it does not
set entry.dirty = false eagerly — instead push a promise that awaits
_writeToDisk(path, entry) and only clears entry.dirty after that promise
resolves successfully (and restore/set dirty on rejection if needed), and apply
the same pattern for _tagDirty so that _tagDirty is only cleared after the
corresponding _fs.write or _fs.delete Promise (from _pTag and _tag) resolves
successfully; reference _flush, _cache, entry.dirty, _writeToDisk, _tagDirty,
_fs.write, _fs.delete, _pTag and _tag when making the change.
In `@src/core/features/notes/controller/NotesController.filters.test.ts`:
- Around line 17-24: The test uses non-standard locale-sensitive date literals
in vi.setSystemTime(...) and new Date(...) which can break across environments;
update all occurrences (the vi.setSystemTime calls in the
NotesController.filters.test and any new Date(...) usages referenced in the
test) to use explicit ISO 8601 UTC strings like "2002-01-01T12:00:00.000Z"
(e.g., replace '01/01/2002 12:00' with '2002-01-01T12:00:00.000Z') so time
setting and parsing are deterministic across runtimes while keeping the same
logical timestamps for notes.add(...) and assertions.
In `@src/core/features/notes/controller/NotesController.ts`:
- Around line 268-277: The code recomputes intersectSets(textMatchCandidates,
filtersCandidates) for the final return instead of reusing the previously
computed results variable; update the no-limit return to use results (e.g.,
Array.from(results) or results.toArray()) so intersectSets is only called once,
preserving the existing pagination logic that uses query.limit and query.page.
In `@src/core/features/notes/controller/NotesTextIndex.ts`:
- Around line 43-57: The session currently fires add/update/remove via
index.addAsync/index.updateAsync/index.removeAsync without tracking their
Promises, so commit (NotesTextIndex.commit) can run before ops finish; modify
the object returned by createIndexSession to maintain an internal pendingOps:
Promise<any>[] array, push each Promise returned by
addAsync/updateAsync/removeAsync inside the session.add/update/remove methods,
and change session.commit to await Promise.all(pendingOps) (handle and propagate
errors) before calling this.commit(), then clear pendingOps after successful
commit to avoid retaining resolved Promises.
In `@src/core/features/notes/controller/NotesTextIndexScanner.test.ts`:
- Around line 31-32: Several test cases call vi.runAllTimersAsync() without
awaiting it, which can leave timer-driven async work incomplete and cause flaky
tests; update each occurrence so the call is awaited (i.e., use await
vi.runAllTimersAsync()) wherever you already call vi.setSystemTime(...) followed
by vi.runAllTimersAsync(), including the occurrences around the
vi.setSystemTime('01/01/2000 12:00') calls and the other instances flagged (the
ones at the nearby lines), so that all timer-based async operations finish
before assertions run.
In `@src/core/features/notes/controller/NotesTextIndexScanner.ts`:
- Around line 20-22: In NotesTextIndexScanner replace the truthy check that sets
startDate (currently using initState && initState.lastUpdate) so that a stored 0
timestamp isn't treated as "no state": use a null/undefined check like
initState?.lastUpdate != null to initialize startDate (new
Date(initState.lastUpdate + 1)) when lastUpdate is 0, ensuring epoch values
produce a valid cursor rather than null.
- Around line 23-47: The scanner can skip notes when many share the same
updatedAt because it advances startDate = lastTimestamp + 1 and sorts only by
updatedAt; modify NotesTextIndexScanner to use a stable tie-breaker and persist
it: change the fetch/sort to sort by {updatedAt: 'asc', id: 'asc'} (or
equivalent in notes.get) and store both lastUpdateTimestamp and lastProcessedId
in this.state via this.state.set; when querying, fetch items with updatedAt >=
lastUpdateTimestamp and for items with updatedAt === lastUpdateTimestamp only
include those with id > lastProcessedId (or use the combined sort and a cursor
of [timestamp,id]) so next page begins after the last processed item instead of
skipping items with identical timestamps.
---
Nitpick comments:
In `@src/core/features/index/flexsearch/FlexSearchStorage.test.ts`:
- Around line 6-25: Tests share a single InMemoryFS instance (indexFs) at module
scope causing order-dependent failures; make each test use a fresh storage by
creating a new InMemoryFS for each test (e.g., move indexFs = new InMemoryFS()
into a beforeEach or instantiate it inside each test) and ensure Index.mount(new
FlexSearchStorage(indexFs)) uses that fresh instance so "Fill the index" no
longer affects "Reuse the index".
In `@src/core/features/index/flexsearch/FlexSearchStorage.ts`:
- Around line 1155-1161: The code uses a hard-to-read comma-operator pattern
when ensuring arrays exist in maps: (map.get(k) ?? (map.set(k, []),
map.get(k)!)). Replace this with a clear helper or explicit assignment: create
or use a helper like getOrInit(map, key) or inline: let arr = map.get(key); if
(!arr) { arr = []; map.set(key, arr); } arr.push(id). Apply this change where
shardOf(id, this._shards.store) and shardOf(id, this._shards.reg) are used with
storeRm and regRm to improve readability and maintain behavior.
- Around line 715-863: The commit() method is large and contains 9 phases;
extract each phase into small private helpers to improve readability and
testability. Create methods like _commitDeletions(fs), _commitMap(fs,
pendingRefs), _commitCtx(fs, pendingRefs), _commitRefs(pendingRefs),
_commitStore(fs), _commitRegistry(fs), _commitTags(fs), _clearAccumulators(fs)
and keep the final _flush() call; preserve the original behavior and ordering
(including Promise.all shard preloads and awaits), pass/return pendingRefs where
needed, and ensure you call the existing helpers used inside commit (e.g.,
_groupBy, _mapShard, _mapSync, _mergeBkts, _markDirty, _ctxShard, _ctxSync,
_refShard, _refSync, _storeShard, _storeSync, _regShard, _regSync) so logic and
concurrency remain identical.
In `@src/core/features/notes/bin/DeletedNotesController.ts`:
- Around line 40-47: The query building in DeletedNotesController passes
updatedAt: { to: undefined } when considerModificationTime is false, which can
be ambiguous for downstream query builders; change the code to construct the
filter object conditionally so that updatedAt is only included when
considerModificationTime is true (e.g., build a base filter with deletedAt and
then add updatedAt: { to: new Date(validityDate) } only when
considerModificationTime is true) before calling notes.query(...) so notes.query
receives no updatedAt key when it shouldn’t be applied.
In `@src/core/features/notes/controller/__tests__/NotesController.bench.ts`:
- Around line 156-172: The benchmark currently calls indexScanner.update()
inside the bench callback (in the bench labelled 'Search note with random
text'), which causes each iteration to re-index and measures indexing+search;
move the indexing step out of the per-iteration workload by running
indexScanner.update() once in a setup phase (e.g., before the bench or in a
beforeAll/benchmark setup) so that the bench callback only calls notes.get({
search: { text: 'powr' }, limit: 10 }) to measure pure search performance.
In `@src/core/features/notes/controller/NotesController.test.ts`:
- Around line 806-847: The test file contains a large commented-out block
testing lexeme index actuality (mentions NotesController, LexemesRegistry,
lexemes.getList(), lexemes.index(), lexemes.prune(), registry.update(),
registry.get()) which should not remain as dead code; either remove the
commented test entirely or convert it into a tracked TODO by creating a short
task/issue in your tracker and replace the commented block with a one-line TODO
that references the new issue ID and a brief reason (e.g., "restore lexeme index
actuality test — tracked in ISSUE-123"); ensure the TODO references
NotesController/LexemesRegistry so it’s discoverable.
In `@src/core/features/notes/controller/NotesTextIndex.ts`:
- Around line 60-65: The query method in NotesTextIndex.ts unsafely casts
index.searchAsync(search) to string[]; instead enforce the string-ID contract at
runtime or return a safe union: call getIndex() then await
index.searchAsync(search) and validate each Id from searchAsync is a string (or
map number->string) before returning, otherwise throw a descriptive error;
alternatively change the method signature to return (string|number)[] if you
want to accept numeric IDs. Reference: query, getIndex, searchAsync and the
NotesTextIndexScanner that inserts string IDs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2cf0e72-4eb7-4650-915a-401c8202edc8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
package.jsonsrc/core/features/files/AsyncState.tssrc/core/features/files/StateFile.tssrc/core/features/index/flexsearch/FlexSearchStorage.test.tssrc/core/features/index/flexsearch/FlexSearchStorage.tssrc/core/features/notes/bin/DeletedNotesController.test.tssrc/core/features/notes/bin/DeletedNotesController.tssrc/core/features/notes/controller/LexemesRegistry.test.tssrc/core/features/notes/controller/LexemesRegistry.tssrc/core/features/notes/controller/NotesController.filters.test.tssrc/core/features/notes/controller/NotesController.test.tssrc/core/features/notes/controller/NotesController.tssrc/core/features/notes/controller/NotesTextIndex.test.tssrc/core/features/notes/controller/NotesTextIndex.tssrc/core/features/notes/controller/NotesTextIndexScanner.test.tssrc/core/features/notes/controller/NotesTextIndexScanner.tssrc/core/features/notes/controller/__tests__/NotesController.bench.tssrc/core/features/notes/controller/index.ts
💤 Files with no reviewable changes (2)
- src/core/features/notes/controller/LexemesRegistry.test.ts
- src/core/features/notes/controller/LexemesRegistry.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This was referenced Apr 3, 2026
This was referenced May 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #262
Key changes
sql.js- an SQLite compiled to a WebAssemblyChores
worker-loadercomlinkto interact with all workersTODOs
Create issues
Summary by CodeRabbit
New Features
Chores