chore: improve dev setup (1)#1987
Conversation
Greptile SummaryThis PR consolidates all dev/test database infrastructure into
Confidence Score: 5/5Safe to merge; all changes are dev/test infrastructure with no impact on the production Electron bundle. The only notable finding is an unquoted $(pwd) in the db:dev npm script — a minor shell-quoting issue that surfaces only when the repo is cloned into a path with spaces. All other changes (fixture pipeline, isolated sqlite alias, postinstall error-handling, ESLint boundary rule) are well-structured and self-consistent. No files require special attention beyond the one-line db:dev quoting fix in package.json.
|
| Filename | Overview |
|---|---|
| package.json | Adds db:dev, db:dev:reset, db:setup, db:fixtures, and test:migrations scripts; db:dev has a minor shell-quoting issue with $(pwd) that breaks paths containing spaces. |
| scripts/postinstall.ts | Adds npm install for tooling/node-deps with correct error and exit-code checking, mirroring the existing runElectronRebuild pattern. |
| tooling/utils/db.ts | New openFixture() helper — uses crypto.randomUUID() for temp-path uniqueness, handles empty and named fixtures cleanly, and deletes WAL/SHM files on close(). |
| tooling/generate-fixtures.ts | Fixture generator wrapped in Vitest describe/it blocks; correctly checkpoints WAL before closing, tracks connections for afterAll cleanup, and splices closed connections to prevent double-close. |
| vitest.config.ts | Adds fixtures and migrations Vitest projects using toolingAlias (system-Node better-sqlite3); node project correctly excludes src/main/db/tests/migrations/** to match the migrations include path. |
| src/main/db/initialize.ts | initializeDatabase() now accepts an optional explicit connection via parameter; lazily imports the app singleton only when no connection is provided, keeping the module importable in non-Electron environments. |
| eslint.config.ts | Adds no-restricted-imports rule blocking @tooling imports in production source trees; test files (*.test.ts) are correctly exempted via ignores. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[pnpm install] -->|postinstall| B[npm install tooling/node-deps]
B --> C[better-sqlite3\ncompiled for system Node]
D[pnpm run db:dev] -->|mkdir -p tooling/db| E[electron-vite dev\nEMDASH_DB_FILE=tooling/db/dev.db]
F[pnpm run db:fixtures] --> G[Vitest: fixtures project]
G -->|toolingAlias| C
G --> H[generate-fixtures.ts\ndescribe/it blocks]
H -->|initializeDatabase + seed| I[tooling/fixtures/*.db\ncommitted snapshots]
J[pnpm run test:migrations] --> K[Vitest: migrations project]
K -->|toolingAlias| C
K --> L[src/main/db/tests/migrations/*.test.ts]
L -->|openFixture| M[copy fixture to os.tmpdir UUID]
M -->|initializeDatabase| N[DrizzleClient\nisolated per test]
N -->|close| O[delete tmp file]
I -.->|source for| M
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
package.json:16
The `$(pwd)` expansion is subject to word splitting if the repository is checked out in a path that contains spaces (e.g. `/Users/alice/my projects/emdash`). The shell treats the unquoted result as multiple words, so `EMDASH_DB_FILE=` gets only the first segment and `electron-vite` receives stray path fragments as positional arguments, producing a confusing "command not found" or "unknown argument" failure. Quoting the substitution prevents the split.
```suggestion
"db:dev": "mkdir -p tooling/db && EMDASH_DB_FILE=\"$(pwd)/tooling/db/dev.db\" electron-vite dev",
```
Reviews (3): Last reviewed commit: "fix: migration tests path" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| spawnSync('npm', ['install', '--prefix', 'tooling/node-deps'], { | ||
| stdio: 'inherit', | ||
| cwd: path.resolve(__dirname, '..'), | ||
| }); |
There was a problem hiding this comment.
spawnSync result for the tooling install is never checked. If npm install --prefix tooling/node-deps fails (network outage, missing C++ build tools, permissions), the process exits cleanly with code 0 while tooling/node-deps/node_modules/better-sqlite3 is absent. The next pnpm run test:migrations or pnpm run db:fixtures then fails with a confusing module-resolution error pointing at the alias path rather than a clear "setup failed" message. The existing runElectronRebuild helper below correctly handles both result.error and result.status !== 0 — apply the same pattern here.
| spawnSync('npm', ['install', '--prefix', 'tooling/node-deps'], { | |
| stdio: 'inherit', | |
| cwd: path.resolve(__dirname, '..'), | |
| }); | |
| const toolingInstall = spawnSync('npm', ['install', '--prefix', 'tooling/node-deps'], { | |
| stdio: 'inherit', | |
| cwd: path.resolve(__dirname, '..'), | |
| }); | |
| if (toolingInstall.error) { | |
| console.error('postinstall: failed to run npm install for tooling/node-deps:', toolingInstall.error); | |
| process.exit(1); | |
| } | |
| if (typeof toolingInstall.status === 'number' && toolingInstall.status !== 0) { | |
| process.exit(toolingInstall.status); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/postinstall.ts
Line: 10-13
Comment:
The `spawnSync` result for the tooling install is never checked. If `npm install --prefix tooling/node-deps` fails (network outage, missing C++ build tools, permissions), the process exits cleanly with code 0 while `tooling/node-deps/node_modules/better-sqlite3` is absent. The next `pnpm run test:migrations` or `pnpm run db:fixtures` then fails with a confusing module-resolution error pointing at the alias path rather than a clear "setup failed" message. The existing `runElectronRebuild` helper below correctly handles both `result.error` and `result.status !== 0` — apply the same pattern here.
```suggestion
const toolingInstall = spawnSync('npm', ['install', '--prefix', 'tooling/node-deps'], {
stdio: 'inherit',
cwd: path.resolve(__dirname, '..'),
});
if (toolingInstall.error) {
console.error('postinstall: failed to run npm install for tooling/node-deps:', toolingInstall.error);
process.exit(1);
}
if (typeof toolingInstall.status === 'number' && toolingInstall.status !== 0) {
process.exit(toolingInstall.status);
}
```
How can I resolve this? If you propose a fix, please make it concise.| "dev:main": "electron-vite dev --watch main", | ||
| "dev:renderer": "electron-vite dev --watch renderer", | ||
| "db:generate": "drizzle-kit generate", | ||
| "db:dev": "EMDASH_DB_FILE=$(pwd)/tooling/db/dev.db electron-vite dev", |
There was a problem hiding this comment.
tooling/db/ is gitignored so it won't exist after a fresh clone. better-sqlite3 creates the database file but does not create missing parent directories — the Electron app will crash at startup with SQLITE_CANTOPEN on first run of db:dev. Adding a mkdir -p tooling/db && prefix ensures the directory exists before the app tries to open the file.
| "db:dev": "EMDASH_DB_FILE=$(pwd)/tooling/db/dev.db electron-vite dev", | |
| "db:dev": "mkdir -p tooling/db && EMDASH_DB_FILE=$(pwd)/tooling/db/dev.db electron-vite dev", |
Prompt To Fix With AI
This is a comment left during a code review.
Path: package.json
Line: 16
Comment:
`tooling/db/` is gitignored so it won't exist after a fresh clone. `better-sqlite3` creates the database file but does not create missing parent directories — the Electron app will crash at startup with `SQLITE_CANTOPEN` on first run of `db:dev`. Adding a `mkdir -p tooling/db &&` prefix ensures the directory exists before the app tries to open the file.
```suggestion
"db:dev": "mkdir -p tooling/db && EMDASH_DB_FILE=$(pwd)/tooling/db/dev.db electron-vite dev",
```
How can I resolve this? If you propose a fix, please make it concise.| name: 'empty' | 'baseline' | (string & {}) = 'baseline' | ||
| ): Promise<FixtureDb> { | ||
| // Create a temp file so each test gets an isolated writable copy. | ||
| const tmpPath = path.join(os.tmpdir(), `emdash-test-${name}-${Date.now()}.db`); |
There was a problem hiding this comment.
Date.now() produces the same value for calls made within the same millisecond. If two tests open the same fixture in rapid succession (e.g., parallel test runners, or two it blocks calling openFixture in the same tick), they'll try to write to the same temp path and one will corrupt the other. crypto.randomUUID() guarantees uniqueness across all concurrent calls.
| const tmpPath = path.join(os.tmpdir(), `emdash-test-${name}-${Date.now()}.db`); | |
| const tmpPath = path.join(os.tmpdir(), `emdash-test-${name}-${crypto.randomUUID()}.db`); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tooling/utils/db.ts
Line: 44
Comment:
`Date.now()` produces the same value for calls made within the same millisecond. If two tests open the same fixture in rapid succession (e.g., parallel test runners, or two `it` blocks calling `openFixture` in the same tick), they'll try to write to the same temp path and one will corrupt the other. `crypto.randomUUID()` guarantees uniqueness across all concurrent calls.
```suggestion
const tmpPath = path.join(os.tmpdir(), `emdash-test-${name}-${crypto.randomUUID()}.db`);
```
How can I resolve this? If you propose a fix, please make it concise.
| /** | ||
| * Example migration test — template for future migration authors. | ||
| * | ||
| * Pattern for writing a migration test: | ||
| * |
There was a problem hiding this comment.
example.test.ts is placed at src/main/db/tests/migrations/ but the migrations Vitest project includes only src/main/db/__tests__/migrations/**/*.test.ts (note double-underscore). This means pnpm run test:migrations finds zero test files and exits silently as if all is well.
The problem goes further: the node Vitest project includes all of src/**/*.test.ts and its exclude list targets src/main/db/__tests__/migrations/** (which also won't match tests/). So the example test is picked up by the node project — which uses the Electron-compiled better-sqlite3 — and will throw a NODE_MODULE_VERSION mismatch at runtime.
The fix is to move the file (and establish the convention) under src/main/db/__tests__/migrations/. The __tests__ prefix additionally benefits from the existing **/_*/** blanket exclude on the node project, so no extra entry in exclude is needed. The agents/risky-areas/database.md docs and the JSDoc inside the file itself already reference __tests__; only the actual path is wrong.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/main/db/tests/migrations/example.test.ts
Line: 1-5
Comment:
**Test file is in the wrong directory and will never run as a migration test**
`example.test.ts` is placed at `src/main/db/tests/migrations/` but the `migrations` Vitest project includes only `src/main/db/__tests__/migrations/**/*.test.ts` (note double-underscore). This means `pnpm run test:migrations` finds zero test files and exits silently as if all is well.
The problem goes further: the `node` Vitest project includes all of `src/**/*.test.ts` and its exclude list targets `src/main/db/__tests__/migrations/**` (which also won't match `tests/`). So the example test is picked up by the `node` project — which uses the Electron-compiled `better-sqlite3` — and will throw a `NODE_MODULE_VERSION` mismatch at runtime.
The fix is to move the file (and establish the convention) under `src/main/db/__tests__/migrations/`. The `__tests__` prefix additionally benefits from the existing `**/_*/**` blanket exclude on the `node` project, so no extra entry in `exclude` is needed. The `agents/risky-areas/database.md` docs and the JSDoc inside the file itself already reference `__tests__`; only the actual path is wrong.
How can I resolve this? If you propose a fix, please make it concise.| exclude: [ | ||
| '**/_*/**', | ||
| 'src/renderer/tests/browser/**', | ||
| 'src/main/db/__tests__/migrations/**', | ||
| ], |
There was a problem hiding this comment.
node project self-consistent with where the test file currently lives, this should either be updated to tests or (better) the file should be moved to __tests__. The node project's existing **/_*/** glob already handles __tests__ — but does not cover tests/, so the current exclude entry is also wrong for the actual path on disk. If the decision is to keep tests/ as the directory name, both the exclude here and the include on the migrations project must be updated.
| exclude: [ | |
| '**/_*/**', | |
| 'src/renderer/tests/browser/**', | |
| 'src/main/db/__tests__/migrations/**', | |
| ], | |
| exclude: [ | |
| '**/_*/**', | |
| 'src/renderer/tests/browser/**', | |
| ], |
Prompt To Fix With AI
This is a comment left during a code review.
Path: vitest.config.ts
Line: 35-39
Comment:
To make the exclude on the `node` project self-consistent with where the test file currently lives, this should either be updated to `tests` or (better) the file should be moved to `__tests__`. The `node` project's existing `**/_*/**` glob already handles `__tests__` — but does **not** cover `tests/`, so the current exclude entry is also wrong for the actual path on disk. If the decision is to keep `tests/` as the directory name, both the `exclude` here and the `include` on the `migrations` project must be updated.
```suggestion
exclude: [
'**/_*/**',
'src/renderer/tests/browser/**',
],
```
How can I resolve this? If you propose a fix, please make it concise.
1 participant
@tooling