feat(db): SQLite foundation — connection, schema, repository, reindex, sync (T4) - #5
Merged
Conversation
…ndex, sync (T4) Phase 1 — connection & migrations - openDatabase / closeDatabase with auto .dennoh/ creation, foreign_keys ON, DELETE journal mode (no WAL — mobile clients open the .db directly so -wal/-shm sidecars are intentionally avoided) - MIGRATIONS: Record<number, Migration> with idempotent runMigrations - v1 creates notes + notes_fts (FTS5, content=notes, unicode61 tokenizer, remove_diacritics=0 for CJK / accented Latin) Phase 2 — CRUD & FTS sync - notes ↔ notes_fts sync triggers added to v1 migration (SQLite external- content FTS does NOT auto-sync; this is the documented trigger pattern) - repository: insertNote / updateNote / deleteNote / getNoteById / getAllNotes, each mutation wrapped in db.transaction so trigger work shares the same tx - mapper: toNoteRow / fromNoteRow with JSON column round-trip Phase 3 — reindex & startup diff - reindexAll: DELETE FROM notes (trigger clears FTS), recursive .md walk with .dennoh/ excluded, per-file errors recorded and execution continues - scanAndSync: FS snapshot vs DB diff — INSERT new, UPDATE when mtime > updated_at, DELETE missing files - index.ts trimmed to spec'd public API; internals reachable via direct module imports for tests Covers T4.1–T4.6. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughSQLite データベース層全体を実装し、ボルト内の Markdown ノートの永続化、スキーマ移行、CRUD 操作、ファイルシステム同期、一括再インデックスを提供します。 ChangesSQLite Database Layer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/db/reindex.ts`:
- Around line 19-32: In walkMdFiles (function walkMdFiles in src/db/reindex.ts
lines 19-32) add try/catch around the async fs.promises.readdir and around
recursive calls so any I/O error for a directory is caught, push an object
describing the failed path and error into a shared errors collection (or yield a
sentinel/error entry) and continue walking other entries instead of throwing; in
the sibling site (src/db/sync.ts lines 20-35) update walkMdFilesWithStats to
catch per-file readdir/stat errors (wrap fs.promises.readdir and
fs.promises.stat calls), accumulate failures into SyncResult.errors (use a
consistent error shape with path and error message), and ensure the function
continues processing remaining files so the aggregate sync flow does not abort
on single I/O failures.
In `@src/db/schema.ts`:
- Around line 96-103: The code currently only handles currentVersion <=
targetVersion; add a fail-fast check in runMigrations: after computing
targetVersion and currentVersion (from getCurrentVersion and MIGRATIONS), if
currentVersion > targetVersion throw a clear Error (or return a rejected
Promise) indicating the DB schema is newer than the binary and migration cannot
proceed; keep existing while loop for currentVersion < targetVersion unchanged.
Ensure the error message includes both currentVersion and targetVersion to aid
debugging.
In `@tests/db/connection.test.ts`:
- Around line 31-36: Wrap the test body that uses openDatabase and closeDatabase
in a try/finally to guarantee cleanup: call const db = openDatabase(vaultPath);
then use try { const row = db.query<{ answer: number }, []>("SELECT 1 AS
answer").get(); expect(row?.answer).toBe(1); } finally { closeDatabase(db); } so
closeDatabase(db) always runs even if the assertion or query throws.
In `@tests/db/reindex.test.ts`:
- Around line 52-57: Remove the conditional and iterate the two arrays as tuples
so the loop has no runtime guard: replace the for loop over indices that checks
idValue/dateValue with a tuple iteration that pairs elements from ids and dates
(e.g. const pairs = ids.map((id, i) => [id, dates[i]] as const); for (const
[idValue, dateValue] of pairs) { ... }), or alternatively use non‑null
assertions when indexing (const idValue = ids[i]!, const dateValue = dates[i]!)
inside the existing index loop and drop the if; target the variables ids, dates,
idValue and dateValue and remove the throw branch so the test contains no if.
In `@tests/db/sync.test.ts`:
- Around line 57-117: Add a new test case exercising the invalid updated_at and
errors-accumulation path: create two notes using generateId()/writeNote() — one
with a valid updatedAt (e.g. "2026-06-12T10:00:00+09:00") and one with an
invalid updatedAt string (e.g. "not-a-date") using fm({ updatedAt: ... }),
bumpMtime() on both files, then call scanAndSync(); assert that the valid note
is inserted/updated (using getNoteById() / notesCount()), the invalid note does
not increment added/updated, and that result.errors contains an entry for the
invalid file (and scanAndSync continues processing the other file). Also add a
separate small test that a single file with invalid updatedAt yields
result.errors length 1 and does not crash (verify DB unchanged) to cover the
Number.isNaN(updatedAtMs) branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 24d90fcf-aa3d-43e6-a29f-4097ec323e82
📒 Files selected for processing (12)
src/db/connection.tssrc/db/index.tssrc/db/mapper.tssrc/db/reindex.tssrc/db/repository.tssrc/db/schema.tssrc/db/sync.tssrc/db/types.tstests/db/connection.test.tstests/db/reindex.test.tstests/db/repository.test.tstests/db/sync.test.ts
Implemented from a Change Stack AI coding task. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
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 (4)
tests/db/reindex.test.ts (2)
26-29: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
row?.c ?? 0は暗黙のフォールバックです。
COUNT(*)は常に行を返すため、rowがnullになることは想定外です。nullの場合はエラーとして扱うべきです。🛡️ 修正案
function notesCount(db: Database): number { const row = db.query<{ c: number }, []>("SELECT COUNT(*) AS c FROM notes").get(); - return row?.c ?? 0; + if (row === null) { + throw new Error("notesCount: unexpected null row from COUNT(*)"); + } + return row.c; }As per coding guidelines,
src/**/*.tsの「フォールバック禁止:value || 'default'のような暗黙のフォールバックを指摘する」に該当します。ただし、これはテストコードなのでテスト失敗で問題が顕在化するため優先度は低いです。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/db/reindex.test.ts` around lines 26 - 29, The helper notesCount(db: Database) currently uses an implicit fallback "row?.c ?? 0"; replace that with explicit handling since COUNT(*) always returns a row: call db.query(...).get(), check if the returned row is null/undefined and throw an Error (or assert) with a clear message if so, otherwise return row.c; keep the function name notesCount and the same query string to locate the change.Source: Coding guidelines
46-99: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winエラー蓄積経路のテストが不足しています。
walkMdFilesの I/O エラー捕捉ロジック（Lines 27-33 inreindex.ts）と、個別ファイル処理エラーの蓄積（Lines 68-70 inreindex.ts）がテストされていません。例えば：
- 読み取り不可能なディレクトリを含む vault での
reindexAll- 不正な frontmatter を含む
.mdファイルでの処理継続これらのエッジケースをカバーするテストを追加することで、エラーハンドリングの堅牢性を検証できます。
As per coding guidelines,
tests/**/*.tsの「エッジケース・エラー経路のカバレッジが十分か」を満たす必要があります。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/db/reindex.test.ts` around lines 46 - 99, Add tests to exercises reindexAll's error-accumulation paths: (1) create a vault subdirectory with permissions that cause walkMdFiles to throw (use fs.chmodSync to remove read/execute before calling reindexAll and restore after) and assert the returned result.errors contains the I/O error and result.processed is 0; (2) add a test that writes one valid note (via writeNote) and one .md with deliberately broken frontmatter/invalid contents so parsing fails, call reindexAll and assert result.processed counts only the valid file, result.errors includes the per-file parse error, and notesCount(db) reflects only successfully indexed rows; reference the reindexAll and walkMdFiles behaviors when locating code to exercise.Source: Coding guidelines
tests/db/sync.test.ts (1)
29-32: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
row?.c ?? 0は暗黙のフォールバックです。
tests/db/reindex.test.tsと同じパターンです。COUNT(*)は常に行を返すため、nullの場合は想定外のエラーとして扱うべきです。🛡️ 修正案
function notesCount(db: Database): number { const row = db.query<{ c: number }, []>("SELECT COUNT(*) AS c FROM notes").get(); - return row?.c ?? 0; + if (row === null) { + throw new Error("notesCount: unexpected null row from COUNT(*)"); + } + return row.c; }As per coding guidelines, フォールバック禁止に該当しますが、テストコードなので優先度は低いです。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/db/sync.test.ts` around lines 29 - 32, The helper notesCount(db: Database) currently uses an implicit fallback "row?.c ?? 0"; change it to treat a missing row as an unexpected error: after the query in notesCount, check if row is null/undefined and throw a clear Error (e.g. "COUNT(*) returned no row in notesCount") instead of returning 0, otherwise return row.c; update the notesCount function body accordingly so the test fails loudly on unexpected DB behavior (this mirrors the same fix needed for the similar pattern in tests/db/reindex.test.ts).Source: Coding guidelines
src/db/sync.ts (1)
64-71:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
errorsが宣言前に使用されており、TDZ (Temporal Dead Zone) によりランタイムエラーが発生します。Line 64 で
walkMdFilesWithStats(vaultPath, errors)を呼び出していますが、errorsは Line 71 で宣言されています。JavaScript/TypeScript のconst/letは宣言前にアクセスするとReferenceErrorをスローします。🐛 修正案
export async function scanAndSync(db: Database, vaultPath: string): Promise<SyncResult> { // Pull the FS view first so we have a stable snapshot before touching the // DB; reading both then diffing minimizes the time window where external // writes during scan could be missed. + const errors: SyncResult["errors"] = []; const fsFiles = new Map<string, number>(); for await (const file of walkMdFilesWithStats(vaultPath, errors)) { fsFiles.set(file.path, file.mtimeMs); } const dbRows = getAllNotes(db); const dbByPath = new Map(dbRows.map((r) => [r.path, r])); - const errors: SyncResult["errors"] = []; let added = 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/sync.ts` around lines 64 - 71, The code calls walkMdFilesWithStats(vaultPath, errors) before errors is declared, causing a TDZ ReferenceError; fix this by declaring and initializing errors (const errors: SyncResult["errors"] = []) before the for-await loop (i.e., move the errors declaration above the call to walkMdFilesWithStats), then keep passing that errors array into walkMdFilesWithStats and downstream logic (fsFiles population, getAllNotes/dbByPath) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/db/reindex.ts`:
- Around line 40-45: Remove the unnecessary try/catch wrappers around the
recursive asynchronous generator yield* calls: in src/db/reindex.ts (lines
40-45) delete the try { yield* walkMdFiles(full, errors); } catch (...) block
and replace it with a plain yield* walkMdFiles(full, errors); call (no other
changes needed there because errors[] is handled by the callee); likewise in
src/db/sync.ts (lines 41-46) remove the try/catch around yield*
walkMdFilesWithStats(full, errors, statsMap); and use a direct yield*
walkMdFilesWithStats(full, errors, statsMap); call, leaving error collection to
the existing inner logic.
---
Outside diff comments:
In `@src/db/sync.ts`:
- Around line 64-71: The code calls walkMdFilesWithStats(vaultPath, errors)
before errors is declared, causing a TDZ ReferenceError; fix this by declaring
and initializing errors (const errors: SyncResult["errors"] = []) before the
for-await loop (i.e., move the errors declaration above the call to
walkMdFilesWithStats), then keep passing that errors array into
walkMdFilesWithStats and downstream logic (fsFiles population,
getAllNotes/dbByPath) unchanged.
In `@tests/db/reindex.test.ts`:
- Around line 26-29: The helper notesCount(db: Database) currently uses an
implicit fallback "row?.c ?? 0"; replace that with explicit handling since
COUNT(*) always returns a row: call db.query(...).get(), check if the returned
row is null/undefined and throw an Error (or assert) with a clear message if so,
otherwise return row.c; keep the function name notesCount and the same query
string to locate the change.
- Around line 46-99: Add tests to exercises reindexAll's error-accumulation
paths: (1) create a vault subdirectory with permissions that cause walkMdFiles
to throw (use fs.chmodSync to remove read/execute before calling reindexAll and
restore after) and assert the returned result.errors contains the I/O error and
result.processed is 0; (2) add a test that writes one valid note (via writeNote)
and one .md with deliberately broken frontmatter/invalid contents so parsing
fails, call reindexAll and assert result.processed counts only the valid file,
result.errors includes the per-file parse error, and notesCount(db) reflects
only successfully indexed rows; reference the reindexAll and walkMdFiles
behaviors when locating code to exercise.
In `@tests/db/sync.test.ts`:
- Around line 29-32: The helper notesCount(db: Database) currently uses an
implicit fallback "row?.c ?? 0"; change it to treat a missing row as an
unexpected error: after the query in notesCount, check if row is null/undefined
and throw a clear Error (e.g. "COUNT(*) returned no row in notesCount") instead
of returning 0, otherwise return row.c; update the notesCount function body
accordingly so the test fails loudly on unexpected DB behavior (this mirrors the
same fix needed for the similar pattern in tests/db/reindex.test.ts).
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 10d7ceba-7ff5-403e-b10b-1fe1a5697f68
📒 Files selected for processing (6)
src/db/reindex.tssrc/db/schema.tssrc/db/sync.tstests/db/connection.test.tstests/db/reindex.test.tstests/db/sync.test.ts
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/db/sync.ts (1)
54-69:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
errors変数が宣言前に参照されており、実行時にReferenceErrorが発生します。Line 59 で
walkMdFilesWithStats(vaultPath, errors)を呼び出していますが、errorsは Line 66 で宣言されています。これは Temporal Dead Zone (TDZ) エラーであり、scanAndSyncを呼び出すと即座にクラッシュします。
errors配列の宣言をfor awaitループの前に移動してください。🐛 修正案
export async function scanAndSync(db: Database, vaultPath: string): Promise<SyncResult> { // Pull the FS view first so we have a stable snapshot before touching the // DB; reading both then diffing minimizes the time window where external // writes during scan could be missed. + const errors: SyncResult["errors"] = []; const fsFiles = new Map<string, number>(); for await (const file of walkMdFilesWithStats(vaultPath, errors)) { fsFiles.set(file.path, file.mtimeMs); } const dbRows = getAllNotes(db); const dbByPath = new Map(dbRows.map((r) => [r.path, r])); - const errors: SyncResult["errors"] = []; let added = 0; let updated = 0; let deleted = 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/sync.ts` around lines 54 - 69, In scanAndSync move the declaration/initialization of the errors array before it's first used: declare "const errors: SyncResult['errors'] = [];" above the for-await loop so the call to walkMdFilesWithStats(vaultPath, errors) uses a valid array; this keeps the rest of the function (variables added/updated/deleted and dbRows/dbByPath logic) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/db/sync.ts`:
- Around line 54-69: In scanAndSync move the declaration/initialization of the
errors array before it's first used: declare "const errors: SyncResult['errors']
= [];" above the for-await loop so the call to walkMdFilesWithStats(vaultPath,
errors) uses a valid array; this keeps the rest of the function (variables
added/updated/deleted and dbRows/dbByPath logic) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4b42a66d-f5a2-48a9-a34b-96e551775de8
📒 Files selected for processing (2)
src/db/reindex.tssrc/db/sync.ts
This was referenced Jun 14, 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
1 participant
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.
Summary
SQLite データベース層の基盤を一通り実装し、T4.1〜T4.6 をカバー。
接続層 (T4.1)
openDatabase/closeDatabase：.dennoh/の自動作成、PRAGMA foreign_keys=ON.dbを直接開く前提で-wal/-shmサイドカーを発生させないためスキーマ / マイグレーション (T4.2, T4.3)
MIGRATIONS: Record<number, Migration>形式、runMigrationsは冪等notes(id PK, path UNIQUE, created_at, updated_at, source, title, projects_json, tags_json)notes_fts（FTS5,content=notes,unicode61 remove_diacritics 0）notes→notes_fts同期トリガー（AFTER INSERT/UPDATE/DELETE）schema_versionテーブルで適用済みバージョンを記録CRUD (T4.4)
insertNote/updateNote/deleteNote/getNoteById/getAllNotesdb.transactionで包み、トリガーと同一 tx 内で FTS 同期updateNoteは対象行不在時に明示エラーupdated_atの更新は呼び出し側責任（disk 上 frontmatter とのレース回避のためリポジトリでは上書きしない）Mapper
toNoteRow/fromNoteRow：projects/tags⇔ JSON 列の双方向変換、titleの null⇔undefined 変換、sourceのランタイム検証body引数は将来の FTS 本文インデックス用予約Reindex (T4.5)
reindexAll：DELETE FROM notesで全消し（トリガーがnotes_ftsを連動クリア）→ vault 再帰走査 → 各 .md をreadNote→toNoteRow→insertNote.dennoh/を除外errors[]に記録して継続Startup diff sync (T4.6)
scanAndSync：FS スナップショット（path + mtime）と DB 行を突き合わせて add / update / delete を判定mtime > Date.parse(updated_at)で UPDATE、Number.isNaNも UPDATE 扱いCodeRabbit
公開 API (
@/db)openDatabase/closeDatabase/runMigrations/insertNote/updateNote/deleteNote/getNoteById/reindexAll/scanAndSyncNoteRow/IndexStats/ReindexResult/SyncResultmapper,getAllNotes,getCurrentVersion等）は直接モジュール参照のみ可設計上の限界
mtimeと frontmatterupdated_atは別の量。初回 reindex 直後の sync ではmtime > updated_atが真になりやすく、変更がなくても UPDATE が走ることがある（無駄な再読込はあるがデータ損失はない）。完全 no-op 化したい場合は DB にmtime列追加 + last-sync-mtime 比較への設計変更が必要。Test plan
bun test（187 pass / 14 files / 352 expects）bun run lint（Biome clean）bun run typecheck（tsc --noEmit clean）🤖 Generated with Claude Code
Summary by CodeRabbit
新機能
テスト