marcel/wannsee-kram
1
0
Fork 0
Code Issues 14 Pull Requests Actions Packages Projects Releases Wiki Activity

Implement database layer with schema initialization #2

New Issue
Open
opened 2026-05-05 10:56:46 +02:00 by marcel · 8 comments
marcel commented 2026-05-05 10:56:46 +02:00
Owner
Copy Link

Task 2 — Plan reference: docs/superpowers/plans/2026-05-05-erbstuecke-wannsee.md

User story: As a developer, I need a database singleton that initializes the schema on first run so that all routes can read and write data without managing connections.

Acceptance criteria

  • src/lib/db.ts exports getDb() returning a better-sqlite3 Database instance
  • getDb() is idempotent — creates singleton, safe to call from any server module
  • WAL journal mode is enabled (PRAGMA journal_mode = WAL)
  • Foreign keys are enabled (PRAGMA foreign_keys = ON)
  • All 4 tables created if not exists: codes, artikel, artikel_fotos, reservierungen
  • UNIQUE(artikel_id) on reservierungen enforces atomic first-come-first-served reservation
  • ON DELETE CASCADE from codes → reservierungen and artikel → artikel_fotos
  • _resetForTesting() export allows Vitest to reset the singleton between tests
  • All 4 Vitest tests pass (tables created, WAL mode, UNIQUE constraint throws, cascade delete works)

Schema (from spec)

codes(id, code UNIQUE, display_name, created_at)
artikel(id, titel, kategorie NOT NULL, notiz, created_at)
artikel_fotos(id, artikel_id→artikel CASCADE, position, filename, UNIQUE(artikel_id,position))
reservierungen(id, artikel_id UNIQUE→artikel, code_id→codes CASCADE, created_at)

Files to create

  • src/lib/db.ts
  • src/lib/db.test.ts

Depends on: #1 | Size: S | Spec: system-design §3

## Task 2 — Plan reference: `docs/superpowers/plans/2026-05-05-erbstuecke-wannsee.md` **User story:** As a developer, I need a database singleton that initializes the schema on first run so that all routes can read and write data without managing connections. ### Acceptance criteria - [ ] `src/lib/db.ts` exports `getDb()` returning a `better-sqlite3` Database instance - [ ] `getDb()` is idempotent — creates singleton, safe to call from any server module - [ ] WAL journal mode is enabled (`PRAGMA journal_mode = WAL`) - [ ] Foreign keys are enabled (`PRAGMA foreign_keys = ON`) - [ ] All 4 tables created if not exists: `codes`, `artikel`, `artikel_fotos`, `reservierungen` - [ ] `UNIQUE(artikel_id)` on `reservierungen` enforces atomic first-come-first-served reservation - [ ] `ON DELETE CASCADE` from `codes → reservierungen` and `artikel → artikel_fotos` - [ ] `_resetForTesting()` export allows Vitest to reset the singleton between tests - [ ] All 4 Vitest tests pass (tables created, WAL mode, UNIQUE constraint throws, cascade delete works) ### Schema (from spec) ```sql codes(id, code UNIQUE, display_name, created_at) artikel(id, titel, kategorie NOT NULL, notiz, created_at) artikel_fotos(id, artikel_id→artikel CASCADE, position, filename, UNIQUE(artikel_id,position)) reservierungen(id, artikel_id UNIQUE→artikel, code_id→codes CASCADE, created_at) ``` ### Files to create - `src/lib/db.ts` - `src/lib/db.test.ts` **Depends on:** #1 | **Size:** S | **Spec:** system-design §3
marcel added this to the v1.0 — MVP milestone 2026-05-05 10:56:46 +02:00
marcel commented 2026-05-05 11:26:00 +02:00
Author
Owner
Copy Link

👤 Markus Keller — Application Architect

Observations

  • The acceptance criteria correctly specify getDb() as the public API, but the system-design spec (lib/db.ts) shows a pattern where prepared statements are also exported from db.ts (e.g., stmtArtikelAll). The issue does not mention exporting prepared statements — the implementation plan must decide whether db.ts exports raw statements or only the singleton. Doing neither now and adding them per-route later creates import chaos.
  • The _resetForTesting() export is correctly specified. This is the right mechanism to avoid the singleton persisting across Vitest tests. However, the issue does not specify what _resetForTesting() should do exactly: close the current connection and set the module-level variable to null/undefined so the next getDb() call re-initialises from the DATABASE_PATH env var — or return a fresh :memory: instance. These are two different contracts. The test file must clarify this before implementation, not after.
  • DATABASE_PATH must come from process.env.DATABASE_PATH per the system-design spec and deployment plan. The issue does not mention what happens when this env var is missing. Per architecture convention, the module must fail loudly at startup — not silently fall back to a default path. A default path silently persists data somewhere unexpected inside the Docker image layer.
  • WAL mode and foreign keys are specified on the singleton init. The singleton pattern guarantees these PRAGMAs run exactly once per process, which is correct. No per-request connection opening.
  • The spec shows schema created via db.exec(SCHEMA_SQL) with IF NOT EXISTS guards — this is correct and must be a single exec() call with all four tables, not four separate calls. A single exec is atomic in SQLite.
  • The SCHEMA_SQL constant should be a named export from db.ts so db.test.ts can reuse it to build :memory: databases without duplicating the schema string.

Recommendations

  • Export SCHEMA_SQL as a named constant from db.ts. Tests import it directly. No schema duplication, no drift between production schema and test schema.
  • Specify _resetForTesting() behaviour precisely in the implementation: it closes the active connection and sets the module-level singleton variable to null, so the next getDb() call creates a fresh connection to whatever DATABASE_PATH is at call time. Tests must set process.env.DATABASE_PATH = ':memory:' before calling getDb() in their beforeEach.
  • Add a startup guard: if process.env.DATABASE_PATH is absent or empty, throw new Error('DATABASE_PATH environment variable is required') before any SQLite call. This is consistent with the SESSION_SECRET guard pattern in auth.ts.
  • Keep all four CREATE TABLE IF NOT EXISTS statements in a single db.exec(SCHEMA_SQL) call — not split across multiple calls or conditional logic.
  • Do not export prepared statements from this issue's scope. Mark them as a follow-up when routes are implemented. Premature statement exports will need to be refactored as queries are refined.

Open Decisions (omit if none)

  • _resetForTesting() contract — Does it reset the singleton so getDb() re-opens DATABASE_PATH (allowing tests to set :memory: via env), or does it accept a path argument and return a new instance? The first is simpler; the second allows tests to avoid env mutation. Either is workable, but the choice must be decided before the test file is written, as it determines the entire test setup pattern.
## 👤 Markus Keller — Application Architect ### Observations - The acceptance criteria correctly specify `getDb()` as the public API, but the system-design spec (`lib/db.ts`) shows a pattern where prepared statements are also exported from `db.ts` (e.g., `stmtArtikelAll`). The issue does not mention exporting prepared statements — the implementation plan must decide whether `db.ts` exports raw statements or only the singleton. Doing neither now and adding them per-route later creates import chaos. - The `_resetForTesting()` export is correctly specified. This is the right mechanism to avoid the singleton persisting across Vitest tests. However, the issue does not specify what `_resetForTesting()` should do exactly: close the current connection and set the module-level variable to `null`/`undefined` so the next `getDb()` call re-initialises from the `DATABASE_PATH` env var — or return a fresh `:memory:` instance. These are two different contracts. The test file must clarify this before implementation, not after. - `DATABASE_PATH` must come from `process.env.DATABASE_PATH` per the system-design spec and deployment plan. The issue does not mention what happens when this env var is missing. Per architecture convention, the module must fail loudly at startup — not silently fall back to a default path. A default path silently persists data somewhere unexpected inside the Docker image layer. - WAL mode and foreign keys are specified on the singleton init. The singleton pattern guarantees these PRAGMAs run exactly once per process, which is correct. No per-request connection opening. - The spec shows schema created via `db.exec(SCHEMA_SQL)` with `IF NOT EXISTS` guards — this is correct and must be a single `exec()` call with all four tables, not four separate calls. A single exec is atomic in SQLite. - The `SCHEMA_SQL` constant should be a named export from `db.ts` so `db.test.ts` can reuse it to build `:memory:` databases without duplicating the schema string. ### Recommendations - Export `SCHEMA_SQL` as a named constant from `db.ts`. Tests import it directly. No schema duplication, no drift between production schema and test schema. - Specify `_resetForTesting()` behaviour precisely in the implementation: it closes the active connection and sets the module-level singleton variable to `null`, so the next `getDb()` call creates a fresh connection to whatever `DATABASE_PATH` is at call time. Tests must set `process.env.DATABASE_PATH = ':memory:'` before calling `getDb()` in their `beforeEach`. - Add a startup guard: if `process.env.DATABASE_PATH` is absent or empty, throw `new Error('DATABASE_PATH environment variable is required')` before any SQLite call. This is consistent with the `SESSION_SECRET` guard pattern in `auth.ts`. - Keep all four `CREATE TABLE IF NOT EXISTS` statements in a single `db.exec(SCHEMA_SQL)` call — not split across multiple calls or conditional logic. - Do not export prepared statements from this issue's scope. Mark them as a follow-up when routes are implemented. Premature statement exports will need to be refactored as queries are refined. ### Open Decisions _(omit if none)_ - **`_resetForTesting()` contract** — Does it reset the singleton so `getDb()` re-opens `DATABASE_PATH` (allowing tests to set `:memory:` via env), or does it accept a path argument and return a new instance? The first is simpler; the second allows tests to avoid env mutation. Either is workable, but the choice must be decided before the test file is written, as it determines the entire test setup pattern.
marcel commented 2026-05-05 11:26:19 +02:00
Author
Owner
Copy Link

👤 Felix Brandt — Fullstack Developer

Observations

  • The issue specifies the right outputs (src/lib/db.ts, src/lib/db.test.ts) and the right acceptance criteria. The implementation is straightforward for an S-sized task — all four tables, two PRAGMAs, one singleton, one reset function, four tests. No ambiguity in the SQL itself.
  • The test criteria map directly to testable assertions: it('creates all 4 tables'), it('WAL mode is active'), it('UNIQUE constraint on reservierungen.artikel_id throws'), it('cascade delete from artikel removes artikel_fotos rows'). These are the correct integration tests using :memory: SQLite — no mocking, no approximation.
  • The issue does not specify whether db.test.ts should use beforeEach / afterEach with :memory: databases or use _resetForTesting(). These serve different purposes: beforeEach with a fresh :memory: instance tests the schema in isolation; _resetForTesting() tests the singleton's reset contract. Both are needed, but for different test categories.
  • better-sqlite3 is synchronous. No await anywhere in db.ts. The implementation should make this impossible to misuse by not wrapping anything in async or returning Promises.
  • The plan references Vitest — vitest.config.ts must be present with environment: 'node' to run better-sqlite3 correctly. JSDOM will break the native module. This is a prerequisite that should be confirmed before the first test run.

Recommendations

  • Structure db.test.ts as two groups: (1) schema creation tests — each uses a fresh new Database(':memory:') with db.exec(SCHEMA_SQL) directly, completely isolated from the singleton; (2) singleton tests — use _resetForTesting() in beforeEach with process.env.DATABASE_PATH = ':memory:' to exercise getDb() directly.
  • The WAL mode test: after calling getDb(), run db.pragma('journal_mode') and assert the result equals 'wal'. Note that on :memory: databases, WAL is silently ignored by SQLite — the pragma call succeeds but returns 'memory'. Test WAL on a temp file path if the WAL assertion must be strict.
  • The UNIQUE constraint test: insert one reservierung, then wrap the second insert in expect(() => ...).toThrow(/UNIQUE constraint failed/). Verify the original row is still intact with a follow-up SELECT.
  • The cascade delete test: insert artikel → insert artikel_fotos → delete artikel → assert artikel_fotos count is zero. Keep foreign_keys = ON in the test setup or the cascade will not fire.
  • Name every test as a complete sentence: 'creates codes, artikel, artikel_fotos, and reservierungen tables on init', not 'db init'.

Open Decisions (omit if none)

  • WAL on :memory: databases — SQLite silently ignores WAL for in-memory databases. If the WAL acceptance criterion must be proven by a test, the test either uses a temp file path (slower, requires cleanup) or the criterion is verified by code review only. Which is acceptable?
## 👤 Felix Brandt — Fullstack Developer ### Observations - The issue specifies the right outputs (`src/lib/db.ts`, `src/lib/db.test.ts`) and the right acceptance criteria. The implementation is straightforward for an S-sized task — all four tables, two PRAGMAs, one singleton, one reset function, four tests. No ambiguity in the SQL itself. - The test criteria map directly to testable assertions: `it('creates all 4 tables')`, `it('WAL mode is active')`, `it('UNIQUE constraint on reservierungen.artikel_id throws')`, `it('cascade delete from artikel removes artikel_fotos rows')`. These are the correct integration tests using `:memory:` SQLite — no mocking, no approximation. - The issue does not specify whether `db.test.ts` should use `beforeEach` / `afterEach` with `:memory:` databases or use `_resetForTesting()`. These serve different purposes: `beforeEach` with a fresh `:memory:` instance tests the schema in isolation; `_resetForTesting()` tests the singleton's reset contract. Both are needed, but for different test categories. - `better-sqlite3` is synchronous. No `await` anywhere in `db.ts`. The implementation should make this impossible to misuse by not wrapping anything in `async` or returning Promises. - The plan references Vitest — `vitest.config.ts` must be present with `environment: 'node'` to run `better-sqlite3` correctly. JSDOM will break the native module. This is a prerequisite that should be confirmed before the first test run. ### Recommendations - Structure `db.test.ts` as two groups: (1) schema creation tests — each uses a fresh `new Database(':memory:')` with `db.exec(SCHEMA_SQL)` directly, completely isolated from the singleton; (2) singleton tests — use `_resetForTesting()` in `beforeEach` with `process.env.DATABASE_PATH = ':memory:'` to exercise `getDb()` directly. - The WAL mode test: after calling `getDb()`, run `db.pragma('journal_mode')` and assert the result equals `'wal'`. Note that on `:memory:` databases, WAL is silently ignored by SQLite — the pragma call succeeds but returns `'memory'`. Test WAL on a temp file path if the WAL assertion must be strict. - The UNIQUE constraint test: insert one `reservierung`, then wrap the second insert in `expect(() => ...).toThrow(/UNIQUE constraint failed/)`. Verify the original row is still intact with a follow-up `SELECT`. - The cascade delete test: insert `artikel` → insert `artikel_fotos` → delete `artikel` → assert `artikel_fotos` count is zero. Keep `foreign_keys = ON` in the test setup or the cascade will not fire. - Name every test as a complete sentence: `'creates codes, artikel, artikel_fotos, and reservierungen tables on init'`, not `'db init'`. ### Open Decisions _(omit if none)_ - **WAL on `:memory:` databases** — SQLite silently ignores WAL for in-memory databases. If the WAL acceptance criterion must be proven by a test, the test either uses a temp file path (slower, requires cleanup) or the criterion is verified by code review only. Which is acceptable?
marcel commented 2026-05-05 11:26:37 +02:00
Author
Owner
Copy Link

👤 Tobias Wendt — DevOps & Platform Engineer

Observations

  • db.ts reads process.env.DATABASE_PATH at module load time. In the Docker container, this resolves to /app/db/erbstuecke.db, which is on the named db volume. This is correct — the file persists across docker compose up --build cycles.
  • The issue does not mention the /app/db/ directory creation. SQLite's better-sqlite3 will throw SQLITE_CANTOPEN if the parent directory does not exist at startup. In production, the named volume is mounted at /app/db and Docker creates the mount point, so the directory exists. In a fresh local dev run without the volume, the directory may not exist. A mkdirSync(path.dirname(DATABASE_PATH), { recursive: true }) call before the new Database() call prevents a confusing startup crash in both environments.
  • The Dockerfile (not in this issue's scope, but relevant) will need better-sqlite3 compiled against the correct Node.js ABI. The multi-stage build must run npm ci --omit=dev in the runner stage; better-sqlite3 is a production dependency (not devDependency) since it's used at runtime. Verify package.json places it in dependencies, not devDependencies.
  • The _resetForTesting() export is a test-only API. It must never be called in the production code path. Since SvelteKit server modules are not tree-shaken in the Node adapter build, this function will appear in the production bundle — that is acceptable as long as nothing calls it. A // FOR TESTING ONLY comment is sufficient.
  • SESSION_SECRET startup guard is mentioned in other specs. The same pattern (fail loudly if missing) should apply to DATABASE_PATH in db.ts for consistency with the deployment posture.

Recommendations

  • Add fs.mkdirSync(path.dirname(dbPath), { recursive: true }) before new Database(dbPath) in the singleton init. This is a one-liner that prevents the most common "works on my machine, fails in Docker on first run" failure mode.
  • Confirm better-sqlite3 is in dependencies (not devDependencies) in package.json. The runner stage of the Dockerfile runs npm ci --omit=dev — if better-sqlite3 is a devDep, the production container crashes on startup.
  • Add a startup env check: if (!process.env.DATABASE_PATH) throw new Error('DATABASE_PATH is required'). Fail at module load, not at first getDb() call — the error surface is cleaner and appears immediately in container logs.
  • The backup strategy (nightly copy of the SQLite file via docker run --rm ... alpine tar) depends on WAL checkpoint completing before the copy. better-sqlite3 checkpoints automatically on db.close(). Ensure the SvelteKit Node adapter calls db.close() on graceful shutdown, or trigger a manual checkpoint (db.pragma('wal_checkpoint(TRUNCATE)')) before the backup window. This is out of scope for this issue but should be tracked.
## 👤 Tobias Wendt — DevOps & Platform Engineer ### Observations - `db.ts` reads `process.env.DATABASE_PATH` at module load time. In the Docker container, this resolves to `/app/db/erbstuecke.db`, which is on the named `db` volume. This is correct — the file persists across `docker compose up --build` cycles. - The issue does not mention the `/app/db/` directory creation. SQLite's `better-sqlite3` will throw `SQLITE_CANTOPEN` if the parent directory does not exist at startup. In production, the named volume is mounted at `/app/db` and Docker creates the mount point, so the directory exists. In a fresh local dev run without the volume, the directory may not exist. A `mkdirSync(path.dirname(DATABASE_PATH), { recursive: true })` call before the `new Database()` call prevents a confusing startup crash in both environments. - The Dockerfile (not in this issue's scope, but relevant) will need `better-sqlite3` compiled against the correct Node.js ABI. The multi-stage build must run `npm ci --omit=dev` in the runner stage; `better-sqlite3` is a production dependency (not devDependency) since it's used at runtime. Verify `package.json` places it in `dependencies`, not `devDependencies`. - The `_resetForTesting()` export is a test-only API. It must never be called in the production code path. Since SvelteKit server modules are not tree-shaken in the Node adapter build, this function will appear in the production bundle — that is acceptable as long as nothing calls it. A `// FOR TESTING ONLY` comment is sufficient. - `SESSION_SECRET` startup guard is mentioned in other specs. The same pattern (fail loudly if missing) should apply to `DATABASE_PATH` in `db.ts` for consistency with the deployment posture. ### Recommendations - Add `fs.mkdirSync(path.dirname(dbPath), { recursive: true })` before `new Database(dbPath)` in the singleton init. This is a one-liner that prevents the most common "works on my machine, fails in Docker on first run" failure mode. - Confirm `better-sqlite3` is in `dependencies` (not `devDependencies`) in `package.json`. The runner stage of the Dockerfile runs `npm ci --omit=dev` — if `better-sqlite3` is a devDep, the production container crashes on startup. - Add a startup env check: `if (!process.env.DATABASE_PATH) throw new Error('DATABASE_PATH is required')`. Fail at module load, not at first `getDb()` call — the error surface is cleaner and appears immediately in container logs. - The backup strategy (nightly copy of the SQLite file via `docker run --rm ... alpine tar`) depends on WAL checkpoint completing before the copy. `better-sqlite3` checkpoints automatically on `db.close()`. Ensure the SvelteKit Node adapter calls `db.close()` on graceful shutdown, or trigger a manual checkpoint (`db.pragma('wal_checkpoint(TRUNCATE)')`) before the backup window. This is out of scope for this issue but should be tracked.
marcel commented 2026-05-05 11:26:57 +02:00
Author
Owner
Copy Link

👤 Nora "NullX" Steiner — Security Engineer

Observations

  • The database layer is the foundation that all auth flows depend on. The codes table is the authentication source of truth for family members, and its integrity is enforced entirely by the UNIQUE constraint on code. No additional application-level deduplication is needed — and none should be added (race conditions in pre-checks are a classic vulnerability, per the architecture doc).
  • DATABASE_PATH comes from the environment. If this path is user-controllable in any context (e.g., a misconfigured Docker setup that reads .env from a user-writable location), an attacker could point it at a database they control, pre-populated with a known family_code value. This is a deployment/ops concern, not a code concern — but it is worth documenting as a threat.
  • The _resetForTesting() export is named with a leading underscore convention to signal it is not for production use. This is a reasonable convention. However, if the production SvelteKit application ever had a route that called this (e.g., a leftover debug endpoint), it would wipe all in-memory state. The function should include a runtime guard: if (process.env.NODE_ENV === 'production') throw new Error('_resetForTesting called in production'). This is a one-line defense.
  • The schema itself has no security issues. ON DELETE CASCADE from codes → reservierungen is correct: when a family member's access code is revoked, their reservations are removed atomically by the database. No orphaned reservation row can remain referencing a deleted code.
  • PRAGMA foreign_keys = ON must be applied per-connection in SQLite — it is not persisted. The singleton pattern ensures it runs exactly once per Node.js process lifetime. If any part of the codebase creates a second better-sqlite3 connection (e.g., in a migration script or test helper that doesn't use getDb()), that second connection will have foreign keys disabled by default. All test setup that creates :memory: databases must explicitly enable foreign keys.

Recommendations

  • Add if (process.env.NODE_ENV === 'production') throw new Error('_resetForTesting() must not be called in production') as the first line of _resetForTesting(). Zero runtime cost in tests; prevents catastrophic misuse in production.
  • Document in a code comment above PRAGMA foreign_keys = ON that this must be set per-connection and is not a persistent database setting. Developers who add a second connection later (e.g., a one-off migration script) will miss this without the comment.
  • In db.test.ts, every test that creates its own :memory: database must explicitly call db.pragma('foreign_keys = ON') — do not assume it is the default. The cascade delete test will silently pass even without cascade if foreign keys are off, producing a false green.
  • The code column in the codes table is TEXT NOT NULL UNIQUE. The code generation function (in auth.ts, out of scope for this issue) must use crypto.randomBytes — not Math.random. This is documented in the auth persona but worth flagging here: the database constraint enforces uniqueness, but only crypto.randomBytes ensures codes are not predictable.

Open Decisions (omit if none)

  • DATABASE_PATH pointing to :memory: in tests via env mutation — Tests that set process.env.DATABASE_PATH = ':memory:' and then call _resetForTesting() + getDb() are mutating the process environment. In a parallel Vitest worker setup, this could cause race conditions between test files. Vitest runs each test file in a separate worker by default, so this is safe — but it should be documented as a constraint: db.test.ts must not be run with --pool=forks without understanding this env mutation.
## 👤 Nora "NullX" Steiner — Security Engineer ### Observations - The database layer is the foundation that all auth flows depend on. The `codes` table is the authentication source of truth for family members, and its integrity is enforced entirely by the UNIQUE constraint on `code`. No additional application-level deduplication is needed — and none should be added (race conditions in pre-checks are a classic vulnerability, per the architecture doc). - `DATABASE_PATH` comes from the environment. If this path is user-controllable in any context (e.g., a misconfigured Docker setup that reads `.env` from a user-writable location), an attacker could point it at a database they control, pre-populated with a known `family_code` value. This is a deployment/ops concern, not a code concern — but it is worth documenting as a threat. - The `_resetForTesting()` export is named with a leading underscore convention to signal it is not for production use. This is a reasonable convention. However, if the production SvelteKit application ever had a route that called this (e.g., a leftover debug endpoint), it would wipe all in-memory state. The function should include a runtime guard: `if (process.env.NODE_ENV === 'production') throw new Error('_resetForTesting called in production')`. This is a one-line defense. - The schema itself has no security issues. `ON DELETE CASCADE` from `codes → reservierungen` is correct: when a family member's access code is revoked, their reservations are removed atomically by the database. No orphaned reservation row can remain referencing a deleted code. - `PRAGMA foreign_keys = ON` must be applied per-connection in SQLite — it is not persisted. The singleton pattern ensures it runs exactly once per Node.js process lifetime. If any part of the codebase creates a second `better-sqlite3` connection (e.g., in a migration script or test helper that doesn't use `getDb()`), that second connection will have foreign keys disabled by default. All test setup that creates `:memory:` databases must explicitly enable foreign keys. ### Recommendations - Add `if (process.env.NODE_ENV === 'production') throw new Error('_resetForTesting() must not be called in production')` as the first line of `_resetForTesting()`. Zero runtime cost in tests; prevents catastrophic misuse in production. - Document in a code comment above `PRAGMA foreign_keys = ON` that this must be set per-connection and is not a persistent database setting. Developers who add a second connection later (e.g., a one-off migration script) will miss this without the comment. - In `db.test.ts`, every test that creates its own `:memory:` database must explicitly call `db.pragma('foreign_keys = ON')` — do not assume it is the default. The cascade delete test will silently pass even without cascade if foreign keys are off, producing a false green. - The `code` column in the `codes` table is `TEXT NOT NULL UNIQUE`. The code generation function (in `auth.ts`, out of scope for this issue) must use `crypto.randomBytes` — not `Math.random`. This is documented in the auth persona but worth flagging here: the database constraint enforces uniqueness, but only `crypto.randomBytes` ensures codes are not predictable. ### Open Decisions _(omit if none)_ - **`DATABASE_PATH` pointing to `:memory:` in tests via env mutation** — Tests that set `process.env.DATABASE_PATH = ':memory:'` and then call `_resetForTesting()` + `getDb()` are mutating the process environment. In a parallel Vitest worker setup, this could cause race conditions between test files. Vitest runs each test file in a separate worker by default, so this is safe — but it should be documented as a constraint: `db.test.ts` must not be run with `--pool=forks` without understanding this env mutation.
marcel commented 2026-05-05 11:27:15 +02:00
Author
Owner
Copy Link

👤 Sara Holt — QA Engineer

Observations

  • The four acceptance criteria map cleanly to four integration tests. This is well-scoped. The issue correctly specifies that all four Vitest tests must pass — this is a hard Definition of Done gate, not a "nice to have."
  • The issue specifies tests in src/lib/db.test.ts but does not specify whether these tests exercise the singleton (getDb()) or test the schema SQL in isolation using direct new Database(':memory:') calls. This is an important distinction: testing the schema SQL directly is fast and reliable; testing getDb() additionally verifies singleton behaviour but requires env var management.
  • The WAL mode test is risky on :memory: databases because SQLite ignores the WAL journal mode for in-memory databases and returns 'memory' for the pragma query. A test that asserts pragma('journal_mode') === 'wal' will fail on :memory:. This must be handled by either testing on a temp file, or by accepting that WAL is verified by code review and tested only on real files.
  • The cascade delete test is the highest-value test in this set — it verifies that ON DELETE CASCADE is actually wired up, which requires PRAGMA foreign_keys = ON to be active at test time. A cascade test that passes without foreign keys enabled is a false positive.
  • No test for UNIQUE(artikel_id, position) on artikel_fotos is specified. This constraint matters — duplicate photo positions on the same article would corrupt gallery ordering. It is a gap in the specified test suite.

Recommendations

  • Structure the four required tests as pure schema tests using new Database(':memory:') + db.exec(SCHEMA_SQL) directly, bypassing the singleton. This removes env var complexity from the test setup and makes each test a true unit of the schema definition.
  • For the WAL test specifically: create a temp file with fs.mkdtempSync + a .db filename, open it with new Database(tempPath), apply pragma('journal_mode = WAL'), then assert pragma('journal_mode') returns 'wal'. Clean up in afterEach. This avoids the :memory: WAL limitation.
  • Add a fifth test (beyond the four specified) for UNIQUE(artikel_id, position) on artikel_fotos. Insert two photos for the same artikel_id with the same position and assert the second insert throws UNIQUE constraint failed. This is a spec requirement that has no test coverage in the issue as written.
  • Each test must use afterEach(() => db.close()) to release the SQLite file handle, especially the WAL temp-file test. Leaking handles will cause intermittent test failures on subsequent runs.
  • Test names must be complete sentences: 'creates all four tables: codes, artikel, artikel_fotos, reservierungen', 'enables WAL journal mode', 'throws UNIQUE constraint error on duplicate reservierungen.artikel_id', 'cascade deletes artikel_fotos rows when the parent artikel is deleted'.

Open Decisions (omit if none)

  • Missing UNIQUE(artikel_id, position) test — The spec requires this constraint, but the issue's four specified tests do not cover it. Should this fifth test be added to the Definition of Done for this issue, or tracked as a separate test gap?
## 👤 Sara Holt — QA Engineer ### Observations - The four acceptance criteria map cleanly to four integration tests. This is well-scoped. The issue correctly specifies that all four Vitest tests must pass — this is a hard Definition of Done gate, not a "nice to have." - The issue specifies tests in `src/lib/db.test.ts` but does not specify whether these tests exercise the singleton (`getDb()`) or test the schema SQL in isolation using direct `new Database(':memory:')` calls. This is an important distinction: testing the schema SQL directly is fast and reliable; testing `getDb()` additionally verifies singleton behaviour but requires env var management. - The WAL mode test is risky on `:memory:` databases because SQLite ignores the WAL journal mode for in-memory databases and returns `'memory'` for the pragma query. A test that asserts `pragma('journal_mode') === 'wal'` will fail on `:memory:`. This must be handled by either testing on a temp file, or by accepting that WAL is verified by code review and tested only on real files. - The cascade delete test is the highest-value test in this set — it verifies that `ON DELETE CASCADE` is actually wired up, which requires `PRAGMA foreign_keys = ON` to be active at test time. A cascade test that passes without foreign keys enabled is a false positive. - No test for `UNIQUE(artikel_id, position)` on `artikel_fotos` is specified. This constraint matters — duplicate photo positions on the same article would corrupt gallery ordering. It is a gap in the specified test suite. ### Recommendations - Structure the four required tests as pure schema tests using `new Database(':memory:')` + `db.exec(SCHEMA_SQL)` directly, bypassing the singleton. This removes env var complexity from the test setup and makes each test a true unit of the schema definition. - For the WAL test specifically: create a temp file with `fs.mkdtempSync` + a `.db` filename, open it with `new Database(tempPath)`, apply `pragma('journal_mode = WAL')`, then assert `pragma('journal_mode')` returns `'wal'`. Clean up in `afterEach`. This avoids the `:memory:` WAL limitation. - Add a fifth test (beyond the four specified) for `UNIQUE(artikel_id, position)` on `artikel_fotos`. Insert two photos for the same `artikel_id` with the same `position` and assert the second insert throws `UNIQUE constraint failed`. This is a spec requirement that has no test coverage in the issue as written. - Each test must use `afterEach(() => db.close())` to release the SQLite file handle, especially the WAL temp-file test. Leaking handles will cause intermittent test failures on subsequent runs. - Test names must be complete sentences: `'creates all four tables: codes, artikel, artikel_fotos, reservierungen'`, `'enables WAL journal mode'`, `'throws UNIQUE constraint error on duplicate reservierungen.artikel_id'`, `'cascade deletes artikel_fotos rows when the parent artikel is deleted'`. ### Open Decisions _(omit if none)_ - **Missing UNIQUE(artikel_id, position) test** — The spec requires this constraint, but the issue's four specified tests do not cover it. Should this fifth test be added to the Definition of Done for this issue, or tracked as a separate test gap?
marcel commented 2026-05-05 11:27:30 +02:00
Author
Owner
Copy Link

👤 Elicit — Requirements Engineer

Observations

  • The issue is well-structured for a Size S task. It has a clear user story, concrete acceptance criteria, explicit schema SQL, and a dependency on #1. It meets the Definition of Ready on most counts.
  • One gap in the acceptance criteria: the user story says "all routes can read and write data without managing connections" — but the issue does not specify whether routes will import getDb() and call it per-request, or import pre-prepared statements exported from db.ts. These are two different contracts, and the downstream route implementations (Issues #3+) will be authored against whichever pattern db.ts establishes. The issue should specify the intended import pattern before implementation starts.
  • The _resetForTesting() acceptance criterion is present but underspecified. "Allows Vitest to reset the singleton between tests" is behavioural intent, not a testable criterion. A testable criterion would be: "After _resetForTesting() is called, a subsequent call to getDb() returns a new Database instance, not the previously returned one."
  • The schema SQL in the issue body matches the system-design spec exactly (§3). No contradiction detected. The UNIQUE(artikel_id) on reservierungen and ON DELETE CASCADE entries are correct.
  • The issue is assigned to milestone v1.0 — MVP and depends on #1. No labels are set. A type: feature or area: backend label would improve backlog health but is not a blocker.
  • Size S is appropriate: two files, ~50 lines of implementation, four tests. Estimable, implementable in under a day.

Recommendations

  • Add one sentence to the acceptance criteria specifying the import pattern for consuming routes: "Routes import getDb() from $lib/db and call it at the top of load functions and actions to obtain the singleton instance." This prevents ambiguity when Issue #3 is implemented.
  • Rewrite the _resetForTesting() criterion as: "_resetForTesting() closes the active connection and sets the singleton to null; a subsequent getDb() call creates a new connection to the current DATABASE_PATH." This is now a testable assertion.
  • The issue does not state what DATABASE_PATH defaults to if the env var is absent. Add to acceptance criteria: "If DATABASE_PATH is not set, the module throws at load time with a descriptive error message." This aligns with the startup-guard pattern used elsewhere in the system design.
  • No open questions block implementation. The schema is fully specified, the file paths are clear, and the dependency (#1, presumably the SvelteKit project scaffold) is identified. This issue is ready to implement as-is, with the minor criterion improvements above.
## 👤 Elicit — Requirements Engineer ### Observations - The issue is well-structured for a Size S task. It has a clear user story, concrete acceptance criteria, explicit schema SQL, and a dependency on #1. It meets the Definition of Ready on most counts. - One gap in the acceptance criteria: the user story says "all routes can read and write data without managing connections" — but the issue does not specify whether routes will import `getDb()` and call it per-request, or import pre-prepared statements exported from `db.ts`. These are two different contracts, and the downstream route implementations (Issues #3+) will be authored against whichever pattern `db.ts` establishes. The issue should specify the intended import pattern before implementation starts. - The `_resetForTesting()` acceptance criterion is present but underspecified. "Allows Vitest to reset the singleton between tests" is behavioural intent, not a testable criterion. A testable criterion would be: "After `_resetForTesting()` is called, a subsequent call to `getDb()` returns a new Database instance, not the previously returned one." - The schema SQL in the issue body matches the system-design spec exactly (§3). No contradiction detected. The `UNIQUE(artikel_id)` on `reservierungen` and `ON DELETE CASCADE` entries are correct. - The issue is assigned to milestone `v1.0 — MVP` and depends on #1. No labels are set. A `type: feature` or `area: backend` label would improve backlog health but is not a blocker. - Size S is appropriate: two files, ~50 lines of implementation, four tests. Estimable, implementable in under a day. ### Recommendations - Add one sentence to the acceptance criteria specifying the import pattern for consuming routes: "Routes import `getDb()` from `$lib/db` and call it at the top of load functions and actions to obtain the singleton instance." This prevents ambiguity when Issue #3 is implemented. - Rewrite the `_resetForTesting()` criterion as: "`_resetForTesting()` closes the active connection and sets the singleton to null; a subsequent `getDb()` call creates a new connection to the current `DATABASE_PATH`." This is now a testable assertion. - The issue does not state what `DATABASE_PATH` defaults to if the env var is absent. Add to acceptance criteria: "If `DATABASE_PATH` is not set, the module throws at load time with a descriptive error message." This aligns with the startup-guard pattern used elsewhere in the system design. - No open questions block implementation. The schema is fully specified, the file paths are clear, and the dependency (#1, presumably the SvelteKit project scaffold) is identified. This issue is ready to implement as-is, with the minor criterion improvements above.
marcel commented 2026-05-05 11:27:40 +02:00
Author
Owner
Copy Link

👤 Leonie Voss — UI/UX Design Lead

Observations

  • Issue #2 is a pure backend/data-layer task. There are no UI components, no visual states, no user-facing flows, and no design tokens involved. This is not a UI issue.
  • One indirect UX relevance: the reservierungen.UNIQUE(artikel_id) constraint is the mechanism that produces the "Dieser Artikel wurde soeben von jemand anderem reserviert." conflict message in the gallery. The database constraint here is the foundation of that user-facing error state. If the constraint is not correctly set up, the conflict message can never be correctly triggered — and family members could see stale "Frei" status for an item that is already reserved.
  • The artikel_fotos.UNIQUE(artikel_id, position) constraint directly affects the photo gallery ordering in ArtikelModal. Corrupt or duplicate positions would cause photos to appear in wrong order or disappear. This is a silent data corruption from the user's perspective.

Recommendations

  • No design changes or UI recommendations for this issue. The implementation should focus entirely on correctness of the schema constraints.
  • When the gallery route (presumably a later issue) implements the "already reserved" error state, confirm with the developer that the error originates from catching SQLITE_CONSTRAINT_UNIQUE on the reservierungen insert — not from a pre-check SELECT. The constraint-driven approach is what makes the user-facing error message reliable under concurrent access.
  • Confirm in the component specs that ArtikelModal reads photos ordered by position ASC. The UNIQUE(artikel_id, position) constraint guarantees no gaps or duplicates, but the query must include an ORDER BY position clause or the display order is undefined.
## 👤 Leonie Voss — UI/UX Design Lead ### Observations - Issue #2 is a pure backend/data-layer task. There are no UI components, no visual states, no user-facing flows, and no design tokens involved. This is not a UI issue. - One indirect UX relevance: the `reservierungen.UNIQUE(artikel_id)` constraint is the mechanism that produces the "Dieser Artikel wurde soeben von jemand anderem reserviert." conflict message in the gallery. The database constraint here is the foundation of that user-facing error state. If the constraint is not correctly set up, the conflict message can never be correctly triggered — and family members could see stale "Frei" status for an item that is already reserved. - The `artikel_fotos.UNIQUE(artikel_id, position)` constraint directly affects the photo gallery ordering in `ArtikelModal`. Corrupt or duplicate positions would cause photos to appear in wrong order or disappear. This is a silent data corruption from the user's perspective. ### Recommendations - No design changes or UI recommendations for this issue. The implementation should focus entirely on correctness of the schema constraints. - When the gallery route (presumably a later issue) implements the "already reserved" error state, confirm with the developer that the error originates from catching `SQLITE_CONSTRAINT_UNIQUE` on the `reservierungen` insert — not from a pre-check SELECT. The constraint-driven approach is what makes the user-facing error message reliable under concurrent access. - Confirm in the component specs that `ArtikelModal` reads photos ordered by `position ASC`. The `UNIQUE(artikel_id, position)` constraint guarantees no gaps or duplicates, but the query must include an `ORDER BY position` clause or the display order is undefined.
marcel commented 2026-05-05 11:28:01 +02:00
Author
Owner
Copy Link

🗳️ Decision Queue — Action Required

5 decisions need your input before implementation starts.

Singleton Reset Contract

  • _resetForTesting() behaviour — Option A: function closes the active connection, sets module-level singleton to null, and the next getDb() call reopens process.env.DATABASE_PATH (tests set this to ':memory:' in beforeEach). Option B: function accepts an optional path argument, creates and returns a fresh instance, replacing the singleton. Option A avoids an extra argument but requires env mutation in tests; Option B is explicit but slightly more complex. (Raised by: Markus Keller, Nora Steiner)

WAL Mode Test Strategy

  • How to test WAL journal mode — Option A: use a temp file path (fs.mkdtempSync) for the WAL test only; assert pragma('journal_mode') returns 'wal'; clean up in afterEach. Option B: skip a live WAL assertion in tests and verify by code review only, since SQLite silently ignores WAL for :memory: databases. Option A is strict and trustworthy; Option B is simpler but leaves a gap. (Raised by: Felix Brandt, Sara Holt)

Missing Test Coverage

  • Fifth test for UNIQUE(artikel_id, position) on artikel_fotos — The four specified acceptance-criteria tests do not cover this constraint. Option A: add it to the Definition of Done for this issue (5 tests total). Option B: track it as a separate test-gap issue for a later cleanup. The constraint is load-bearing for gallery photo ordering. (Raised by: Sara Holt)

Startup Guards

  • DATABASE_PATH missing at startup — Should db.ts throw immediately at module load if process.env.DATABASE_PATH is absent, or silently fall back to a default path? Throwing is consistent with the SESSION_SECRET guard pattern and surfaces the problem in container logs immediately. Falling back hides misconfiguration. (Raised by: Markus Keller, Tobias Wendt, Elicit)

Vitest Environment Config

  • environment: 'node' in vitest.config.ts — better-sqlite3 is a native Node module and will fail under JSDOM. vitest.config.ts must set test.environment = 'node'. This is a prerequisite, not a decision — but it must be confirmed as part of the Issue #1 scaffold before this issue's tests can run. If vitest.config.ts was not included in Issue #1, it needs to be added here. (Raised by: Felix Brandt)
## 🗳️ Decision Queue — Action Required _5 decisions need your input before implementation starts._ ### Singleton Reset Contract - **`_resetForTesting()` behaviour** — Option A: function closes the active connection, sets module-level singleton to `null`, and the next `getDb()` call reopens `process.env.DATABASE_PATH` (tests set this to `':memory:'` in `beforeEach`). Option B: function accepts an optional path argument, creates and returns a fresh instance, replacing the singleton. Option A avoids an extra argument but requires env mutation in tests; Option B is explicit but slightly more complex. _(Raised by: Markus Keller, Nora Steiner)_ ### WAL Mode Test Strategy - **How to test WAL journal mode** — Option A: use a temp file path (`fs.mkdtempSync`) for the WAL test only; assert `pragma('journal_mode')` returns `'wal'`; clean up in `afterEach`. Option B: skip a live WAL assertion in tests and verify by code review only, since SQLite silently ignores WAL for `:memory:` databases. Option A is strict and trustworthy; Option B is simpler but leaves a gap. _(Raised by: Felix Brandt, Sara Holt)_ ### Missing Test Coverage - **Fifth test for `UNIQUE(artikel_id, position)` on `artikel_fotos`** — The four specified acceptance-criteria tests do not cover this constraint. Option A: add it to the Definition of Done for this issue (5 tests total). Option B: track it as a separate test-gap issue for a later cleanup. The constraint is load-bearing for gallery photo ordering. _(Raised by: Sara Holt)_ ### Startup Guards - **`DATABASE_PATH` missing at startup** — Should `db.ts` throw immediately at module load if `process.env.DATABASE_PATH` is absent, or silently fall back to a default path? Throwing is consistent with the `SESSION_SECRET` guard pattern and surfaces the problem in container logs immediately. Falling back hides misconfiguration. _(Raised by: Markus Keller, Tobias Wendt, Elicit)_ ### Vitest Environment Config - **`environment: 'node'` in `vitest.config.ts`** — `better-sqlite3` is a native Node module and will fail under JSDOM. `vitest.config.ts` must set `test.environment = 'node'`. This is a prerequisite, not a decision — but it must be confirmed as part of the Issue #1 scaffold before this issue's tests can run. If `vitest.config.ts` was not included in Issue #1, it needs to be added here. _(Raised by: Felix Brandt)_
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
Compat/Breaking
Breaking change that won't be backward compatible
 Kind/Bug
Something is not working
 Kind/Documentation
Documentation changes
 Kind/Enhancement
Improve existing functionality
 Kind/Feature
New functionality
 Kind/Security
This is security issue
 Kind/Testing
Issue or pull request related to testing
Priority
Critical
1
The priority is critical
Priority
High
2
The priority is high
Priority
Low
4
The priority is low
Priority
Medium
3
The priority is medium
Reviewed
Confirmed
1
Issue has been confirmed
Reviewed
Duplicate
2
This issue or pull request already exists
Reviewed
Invalid
3
Invalid issue
Reviewed
Won't Fix
3
This issue won't be fixed
Status
Abandoned
3
Somebody has started to work on this but abandoned work
Status
Blocked
1
Something is blocking this issue or pull request
Status
Need More Info
2
Feedback is required to reproduce issue or to continue work
admin
Admin interface feature
 deployment
Docker and hosting configuration
 feature
New user-facing feature
 setup
Project scaffolding and infrastructure
No Label
Milestone
No items
No Milestone v1.0 — MVP
Projects
Clear projects
No project
Assignees
Clear assignees
marcel
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: marcel/wannsee-kram#2
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?