feat(desktop): SQLite persistence + native rebuild infrastructure (W2)#662
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
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:
📝 WalkthroughWalkthroughAdds persistent SQLite-backed settings using better-sqlite3 with migrations (PRAGMA user_version), a run-once legacy JSON import, DB lifecycle wiring in Electron main, native-module packaging and rebuild support (Electron vs Node ABI), Vite/Forge changes to keep/copy native modules, CI test-before-package ordering, and comprehensive tests for DB and settings. ChangesSQLite Settings Storage & Native Module Integration
Sequence DiagramsequenceDiagram
participant App as "Electron App"
participant DB as "SQLite DB"
participant Store as "Settings Store"
participant FS as "Filesystem"
rect rgba(100, 200, 100, 0.5)
App->>App: initDb()
App->>DB: open/create app.db_v1
App->>DB: apply pragmas (WAL, foreign_keys, timeout)
App->>DB: applyMigrations()
DB-->>App: ready
end
rect rgba(100, 150, 200, 0.5)
Store->>FS: check settings.json exists
FS-->>Store: contents or not
alt legacy exists & valid
Store->>DB: INSERT key/value rows
FS->>FS: rename to settings.json.migrated
DB-->>Store: migrated
else missing or invalid
Store-->>Store: skip, use defaults
end
end
rect rgba(200, 150, 100, 0.5)
App->>Store: getSettings()
Store->>DB: SELECT all settings
DB-->>Store: key/value rows
Store->>Store: parse JSON, merge defaults, validate
Store-->>App: snapshot
end
rect rgba(200, 100, 150, 0.5)
App->>Store: updateSetting(key, value)
Store->>Store: validate
Store->>DB: INSERT OR REPLACE
DB-->>Store: ok
Store->>App: notify listeners
end
rect rgba(150, 150, 100, 0.5)
App->>App: closeDb()
App->>DB: close connection
DB-->>App: closed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
[blocker] apps/desktop/forge.config.ts — copyNativeRuntimeModules must copy the native .node artifacts with correct relative layout and file modes (preserve .node files and node_modules structure). If missing or laid out incorrectly the packaged app will fail to load better-sqlite3 at runtime. [warning] apps/desktop/scripts/rebuild-sqlite.mjs & package.json scripts — CI/developers must run the Electron-targeted rebuild against the exact Electron version used for packaging; otherwise ABI mismatch will produce unusable native bindings during dev or packaging. [nit] apps/desktop/src/main/settings-store.ts — renaming legacy 🚥 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 unit tests (beta)
✨ Simplify code
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: 3
🧹 Nitpick comments (2)
apps/desktop/scripts/rebuild-sqlite.mjs (1)
15-19: 💤 Low value[nit] No error handling if Electron isn't installed.
If
electron/package.jsondoesn't exist (e.g., deps not installed),execFileSyncthrows a cryptic error. A try/catch with a clearer message would improve DX, but since this is a dev script and pnpm would fail earlier anyway, it's low priority.🤖 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 `@apps/desktop/scripts/rebuild-sqlite.mjs` around lines 15 - 19, The script reads Electron's version using execFileSync into electronVersion but lacks error handling if 'electron/package.json' isn't present; wrap the execFileSync call that assigns electronVersion in a try/catch, catch the thrown error, log a clear, actionable message via console.error (including the original error.message) that explains Electron isn't installed or deps need installing, and exit non‑zero (e.g., process.exit(1)); update the error handling around the execFileSync invocation so failures are user-friendly.apps/desktop/src/main/db/schema.ts (1)
27-29: 💤 Low value[nit] Dead code —
sqlcan never be undefined here.Given
target <= registry.lengthandsql = registry[target - 1], the index is always in bounds. This guard is unreachable.♻️ Remove dead code
for (let target = current + 1; target <= registry.length; target++) { const sql = registry[target - 1]; - if (sql === undefined) { - continue; - } + // sql is guaranteed defined: target-1 is always in [0, registry.length-1] db.transaction(() => {🤖 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 `@apps/desktop/src/main/db/schema.ts` around lines 27 - 29, The guard checking "if (sql === undefined) { continue; }" is unreachable because sql is assigned as registry[target - 1] with target <= registry.length, so remove that dead check; update the loop in which sql, registry and target are used (the block assigning sql from registry[target - 1]) to eliminate the redundant undefined branch and any related unreachable code paths so the logic proceeds directly with sql.
🤖 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 `@apps/desktop/README.md`:
- Around line 63-70: The commands in this section (pnpm test, pnpm
rebuild:sqlite:node, pnpm rebuild:sqlite) are ambiguous when run from the
monorepo root; update the README to either 1) prefix each command to target the
workspace package (e.g. pnpm -F `@lightfast/desktop` test, pnpm -F
`@lightfast/desktop` rebuild:sqlite:node, pnpm -F `@lightfast/desktop`
rebuild:sqlite) or 2) add a clear sentence that the instructions assume you have
changed directory into the desktop app (cd apps/desktop) before running the
listed commands so readers can run the exact commands as shown.
In `@apps/desktop/src/main/db/index.ts`:
- Around line 44-53: closeDb() currently clears connection but leaves the
initFailed flag set, so subsequent initDb() calls will bail out; update
closeDb() to reset initFailed to false (after closing the connection and before
returning) so initDb() can attempt recovery/reinitialization, referencing the
initFailed variable and the closeDb() and initDb() functions; alternatively, if
single-shot behavior is intended, add a comment in closeDb() documenting that
initFailed is intentionally not reset.
In `@apps/desktop/src/main/settings-store.ts`:
- Around line 149-150: The code updates the in-memory cached variable before
calling writeKey(key, value), which causes cache/DB divergence if writeKey
fails; move the cache update so it only occurs after writeKey succeeds (i.e.,
call writeKey(key, value) first and then set cached = parsed.data), or
alternatively wrap writeKey in a try/catch and only assign to cached on success
and revert/log on failure; update the logic around the cached variable and
writeKey(key, value) (and ensure getSettings() behavior remains consistent) so
the cache is only mutated when the DB write completes successfully.
---
Nitpick comments:
In `@apps/desktop/scripts/rebuild-sqlite.mjs`:
- Around line 15-19: The script reads Electron's version using execFileSync into
electronVersion but lacks error handling if 'electron/package.json' isn't
present; wrap the execFileSync call that assigns electronVersion in a try/catch,
catch the thrown error, log a clear, actionable message via console.error
(including the original error.message) that explains Electron isn't installed or
deps need installing, and exit non‑zero (e.g., process.exit(1)); update the
error handling around the execFileSync invocation so failures are user-friendly.
In `@apps/desktop/src/main/db/schema.ts`:
- Around line 27-29: The guard checking "if (sql === undefined) { continue; }"
is unreachable because sql is assigned as registry[target - 1] with target <=
registry.length, so remove that dead check; update the loop in which sql,
registry and target are used (the block assigning sql from registry[target - 1])
to eliminate the redundant undefined branch and any related unreachable code
paths so the logic proceeds directly with sql.
🪄 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: CHILL
Plan: Pro
Run ID: 596c4928-4d8a-434c-848e-2a1befa231c2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/pnpm-lock.yaml
📒 Files selected for processing (14)
.github/workflows/desktop-ci.ymlapps/desktop/README.mdapps/desktop/forge.config.tsapps/desktop/package.jsonapps/desktop/scripts/rebuild-sqlite.mjsapps/desktop/src/main/__tests__/db.test.tsapps/desktop/src/main/__tests__/settings-store.test.tsapps/desktop/src/main/__tests__/sqlite-smoke.test.tsapps/desktop/src/main/db/index.tsapps/desktop/src/main/db/schema.tsapps/desktop/src/main/index.tsapps/desktop/src/main/settings-store.tsapps/desktop/vite.main.config.tspnpm-workspace.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/desktop-ci.yml:
- Around line 76-87: The workflow step named "Upload packaged app" currently
uses a mutable tag "actions/upload-artifact@v4"; replace that with the action
pinned to a full commit SHA (e.g., actions/upload-artifact@<full-commit-sha>) so
the action is immutable. Locate the step by the "Upload packaged app" name or
the uses line and update the uses field to the repository@<full-length-SHA> for
actions/upload-artifact; ensure you fetch the exact SHA from the official
actions/upload-artifact repo and commit the change.
🪄 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: CHILL
Plan: Pro
Run ID: 853a028a-69f0-4415-bbc1-c04a5c810ea1
📒 Files selected for processing (1)
.github/workflows/desktop-ci.yml
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/README.md (1)
77-81: 💤 Low value[nit] Minor clarity: "until the file is removed" could be more precise.
Line 78 says settings aren't persisted "until the file is removed" — technically, you'd remove
app.db_v1(the DB), not "the file" (ambiguous). Consider: "untilapp.db_v1is removed."🤖 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 `@apps/desktop/README.md` around lines 77 - 81, Improve clarity in the README sentence by replacing the ambiguous "the file" with the specific database filename; update the sentence referencing initDb() and app.db_v1 to say settings are not persisted "until app.db_v1 is removed" (or similar) so readers know you mean the DB file rather than an unspecified file.apps/desktop/src/main/settings-store.ts (1)
89-110: 💤 Low value[warning] Silent JSON parse failures in
readFromDb()drop individual keys without logging.Lines 103-105 catch malformed JSON silently. If a value gets corrupted, debugging why a setting reverted to default requires inspecting the DB directly. Consider a debug-level log.
Optional: add debug logging for malformed rows
try { partial[row.key] = JSON.parse(row.value); - } catch { + } catch (err) { + logger.debug("[settings] skipping malformed row", { key: row.key, err }); // skip malformed row; defaults will fill the gap }🤖 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 `@apps/desktop/src/main/settings-store.ts` around lines 89 - 110, readFromDb silently swallows JSON.parse errors causing keys to be dropped without trace; update the catch block inside readFromDb to emit a debug-level log that includes the offending key (row.key), the raw string (row.value), and the parse error so you can trace corrupted settings; use the project's existing logger (the same logger used elsewhere in settings-store.ts) to log a clear message before skipping the row so malformed entries can be diagnosed.
🤖 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.
Nitpick comments:
In `@apps/desktop/README.md`:
- Around line 77-81: Improve clarity in the README sentence by replacing the
ambiguous "the file" with the specific database filename; update the sentence
referencing initDb() and app.db_v1 to say settings are not persisted "until
app.db_v1 is removed" (or similar) so readers know you mean the DB file rather
than an unspecified file.
In `@apps/desktop/src/main/settings-store.ts`:
- Around line 89-110: readFromDb silently swallows JSON.parse errors causing
keys to be dropped without trace; update the catch block inside readFromDb to
emit a debug-level log that includes the offending key (row.key), the raw string
(row.value), and the parse error so you can trace corrupted settings; use the
project's existing logger (the same logger used elsewhere in settings-store.ts)
to log a clear message before skipping the row so malformed entries can be
diagnosed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 76eb0384-b317-40d6-86c2-eb0748630f14
📒 Files selected for processing (3)
apps/desktop/README.mdapps/desktop/src/main/db/index.tsapps/desktop/src/main/settings-store.ts
Inserts pnpm --filter @lightfast/desktop test between Typecheck and Package (unsigned). Tests run against the Node-ABI better-sqlite3 prebuilt produced by pnpm install; the subsequent Package step rebuilds the binding to Electron's ABI. Order matters — running tests after package would hit NODE_MODULE_VERSION mismatch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… (W2 phase 1)
Lands the persistence floor without flipping any consumer:
- better-sqlite3 + @types/better-sqlite3; @electron/rebuild devDep
- pnpm-workspace.yaml: better-sqlite3 in onlyBuiltDependencies
- forge.config.ts: rebuildConfig.onlyModules + copyNativeRuntimeModules
in packageAfterCopy hook (vite plugin strips node_modules; AutoUnpack
alone won't surface .node binding without this copy step)
- vite.main.config.ts: better-sqlite3 in rollupOptions.external so
bootstrap.js keeps the require() for runtime resolution
- scripts/rebuild-sqlite.mjs: dual-target ABI rebuild (Electron vs Node)
for the local dev loop
- README: dual-ABI rebuild footgun documented
- src/main/db/{index,schema}.ts: connection wrapper with WAL +
PRAGMA user_version migration registry (one v1 migration creates
the STRICT settings table)
- src/main/index.ts: initDb() after initSentry(); closeDb() on will-quit
- tests: sqlite-smoke + db (init, idempotency, schema, pragmas,
user_version, close)
Phase 1 is dormant from the user's POV — settings-store still reads/writes
JSON. Phase 2 flips the consumer.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… phase 2)
Replaces synchronous readFileSync/writeFileSync over settings.json with
better-sqlite3-backed reads/writes against the settings table from
phase 1. Public API (getSettings, updateSetting, onSettingsChanged,
SettingsSnapshot) is byte-identical so call sites in index.ts:172-230
stay unchanged.
One-shot legacy import on first read: if <userData>/settings.json exists
and the settings table is empty, parse + Zod-validate, INSERT OR IGNORE
each key inside a transaction, then rename to settings.json.migrated.
Idempotent — runs once per machine. Malformed JSON logs + skips,
leaving the file untouched. DB unavailable falls through to defaults
with logged no-op writes.
INSERT OR REPLACE on writes (no FKs / no AUTOINCREMENT, simpler form
fits). Sentry tag scope: settings.{migrate,write}.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds actions/upload-artifact step right after Package (unsigned) so reviewers can pull the packaged tree from each leg and verify native bindings (better_sqlite3.node, future native deps) actually ship under app.asar.unpacked/. Per-OS artifact name (Linux/macOS) prevents matrix collision; if-no-files-found=error makes a silent forge regression loud. Closes the local-only verification gap: macOS .node presence was verified locally during W2 phase 1, but the Linux leg had never been inspected. CI passing the Package step alone doesn't prove the binding shipped — electron-forge doesn't validate runtime requirements. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- README: qualify pnpm commands with -F @lightfast/desktop so they're copy-pastable from the repo root, not ambiguous in the monorepo. - db: reset initFailed in closeDb() so tests/dev hot-reload can re-init. - settings-store: writeKey before cache update, so the cache only advances after the persist attempt has run. Skipped CodeRabbit's "pin actions/upload-artifact to a SHA" suggestion because every other workflow in this repo uses mutable @v4 tags; SHA pinning is a repo-wide policy decision, not in scope for the SQLite PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jeevanpillay
force-pushed
the
feat/desktop-sqlite-persistence
branch
from
2a14d1c to
0a90cc0
Compare
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
settings-store.tsJSON I/O withbetter-sqlite3(<userData>/app.db_v1, WAL, STRICT settings table,PRAGMA user_versionmigration registry); public API unchanged@electron/rebuild, dual-target ABI script,forge.config.tsrebuildConfig +copyNativeRuntimeModulesto work around vite-plugin'snode_modulesstrip)settings.json→ SQLite import on first boot (renames legacy file to.migrated); insertpnpm teststep intodesktop-ci.ymlbetween typecheck and package so vitest runs on Node-ABI prebuilts before electron-forge rebuilds to Electron ABIPlan:
thoughts/shared/plans/2026-05-07-desktop-sqlite-persistence.mdThree commits, one per plan phase (committed out of order — Phase 3 first, then 1+2; identical PR diff):
1b6ad817bPhase 1 — deps, db wrapper, native rebuild infra (dormant)f0963c56bPhase 2 — settings-store SQLite flip + legacy importc4c5b3cfaPhase 3 — CI vitest stepTest plan
pnpm --filter @lightfast/desktop typecheckpnpm --filter @lightfast/desktop test— 69/69 green (db, settings-store, sqlite-smoke + existing suites)pnpm --filter @lightfast/desktop package(macOS arm64) —Lightfast.app/Contents/Resources/app.asar.unpacked/node_modules/better-sqlite3/build/Release/better_sqlite3.nodepresentapp.db_v1(+-shm/-wal) created,PRAGMA user_version=1,.schema settingsmatches v1 migrationsettings.jsonimported into table, file renamed to.migrated,themeSourceround-trips across restart.debartifact containsbetter_sqlite3.nodeunder/opt/Lightfast/resources/app.asar.unpacked/.../build/Release/(verify from CI artifact)desktop-ci.ymlgreen onmacos-14andubuntu-22.04legs (this PR's CI run)<userData>/app.db_v1appears, Settings window shows defaults🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests