Claude finished @Yeraze's task —— View job

PR Review: refactor(test): shared createTestDb helper from migration registry

Tasks:

• Gather context and understand the PR
• Read changed files
• Analyze code quality, bugs, performance, security
• Post review feedback

Summary

This is a well-targeted refactor that solves a real, documented problem. The design is sound and the implementation is clean. A few observations below, ranging from a genuine concern down to minor nits.

Concern: Redundant dedupe index in telemetry.test.ts beforeEach

src/db/repositories/telemetry.test.ts:419–426

The duplicate packet dedupe describe block still manually creates telemetry_source_packet_type_uniq in its own beforeEach:

beforeEach(() => {
  db.exec(`
    CREATE UNIQUE INDEX IF NOT EXISTS telemetry_source_packet_type_uniq
      ON telemetry(sourceId, nodeNum, packetId, telemetryType)
      WHERE packetId IS NOT NULL
  `);
});

But createTestDb() now runs the full migration registry, which includes migration 032 (032_telemetry_packet_dedupe.ts) that already creates this exact index. The IF NOT EXISTS guard means this doesn't cause a test failure, but it's now dead DDL — a reader will reasonably wonder whether the index is present before this block runs, and whether they need to keep this in sync with the migration.

The comment above the describe block (// Regression: issue #3119) says the "migration 032 partial unique index must suppress" conflicts. It should work without the extra db.exec. Consider dropping the inner beforeEach and adding a comment noting the index comes from migration 032 via createTestDb().

Nit: telemetry.extra.test.ts imports still include BetterSQLite3Database but the comment on telemetrySqlite schema import was removed

src/db/repositories/telemetry.extra.test.ts:15-16

The type imports from Drizzle and better-sqlite3 are still present (Database, BetterSQLite3Database) which is fine since the local variables use them. This is correct as-is.

Design observation: no sourceId FK guard in the round-trip smoke test

src/server/test-helpers/testDb.test.ts:30–32

The nodes INSERT in the smoke test uses sourceId: 'default' — worth confirming that sources table has a 'default' row (or that there's no FK enforced from nodes.sourceId → sources.id). In production the baseline migration seeds 'default'; migration 020 creates the sources table and the baseline should seed it. Since createTestDb() runs all migrations including 001, this should be fine — but the smoke test itself doesn't assert it. Low risk, just noting it.

Minor: as any cast in the migration runner

src/server/test-helpers/testDb.ts:59

migration.sqlite(sqlite as any, getSetting, setSetting);

The as any is needed because migrations typed their db argument as Database from better-sqlite3 but Drizzle's SQLite type has a slightly different surface. This is the same workaround used in production. A brief inline comment explaining why the cast is needed (vs. being a lazy bypass) would help future maintainers — though the existing block comment at line 36–38 partially covers it.

Positive highlights

• The helper itself is tight and correct. Running real migrations against :memory: is the right approach — it's fast (sub-millisecond to spin up) and eliminates schema drift by construction.
• The getSetting/setSetting shims are defensive without being fragile — the try/catch handles the bootstrapping window cleanly.
• The smoke test is well-structured: it checks table existence, a column added by a recent migration, and a full repo round-trip. That last test is especially valuable since it exercises the Drizzle binding.
• The three migrated test files are clean drop-ins. The reduction from 34/28/12 lines of hand-rolled DDL to 6-7 lines of setup is a real readability win.
• Idiomatic afterEach(() => t?.close()) pattern in the smoke test prevents connection leaks properly.

Verdict

Approve with the dedupe-index observation. The redundant CREATE UNIQUE INDEX in telemetry.test.ts:419-426 isn't a correctness issue (IF NOT EXISTS saves it), but it's the kind of thing that will confuse the next developer migrating another test file. Worth a one-line cleanup or a clarifying comment before merge.

Claude finished @Yeraze's task —— View job

PR Review: refactor(test): shared createTestDb helper from migration registry

Tasks:

• Gather context and understand the PR
• Read changed files
• Analyze code quality, bugs, performance, security
• Post review feedback

Summary

This is a well-structured, correctly motivated refactor. The previous review comment on the redundant dedupe-index beforeEach has been addressed — the block comment at telemetry.test.ts:419–422 now clearly explains that the index comes from the migration registry, and the beforeEach DDL is gone. The implementation is solid. A few observations below:

Positive highlights

• Root cause fix: Using real migrations against :memory: is definitively correct — the schema drift that caused feat(map): position-history markers at every fix, points-only mode, hover tooltip (#3492) #3495 is structurally impossible with this approach.
• The helper is minimal and correct. The getSetting/setSetting try/catch shims are the right defensive choice for the bootstrapping window, and the settingsKey guard-write at line 63 faithfully replicates production semantics.
• The smoke test covers the right things: table presence, a recently-added column (migration 089 rxSnr), and an end-to-end repository round-trip. Checking rxSnr specifically means future column-add migrations are immediately validated.
• The three migrated test files are clean and reduced. The comment pattern (// Full production schema from the migration registry — see testDb.ts) consistently points readers to the helper.
• The previous concern is addressed. The dedupe beforeEach is gone; the describe block now carries a clear comment that createTestDb() already provides the migration 032 index.

Minor: telemetry.extra.test.ts imports BetterSQLite3Database type but the variable is only typed at declaration

src/db/repositories/telemetry.extra.test.ts:15

import { BetterSQLite3Database } from 'drizzle-orm/better-sqlite3';

This is used at line 35 (let drizzleDb: BetterSQLite3Database<typeof schema>), so it's not dead — this is fine as-is. Just confirming it's intentional.

Observation: testDb.test.ts:30–32 — smoke test inserts into nodes without a sources row

t.sqlite.prepare(
  "INSERT INTO nodes (nodeNum, nodeId, sourceId, createdAt, updatedAt) VALUES (?, ?, ?, ?, ?)",
).run(0xaabbccdd, '!aabbccdd', 'default', Date.now(), Date.now());

This works because the baseline migration (001) seeds a 'default' row into sources, so the FK is satisfied. It's correct — just worth noting that the test relies on that seed behavior. If you ever add an assertion like expect(tables).toContain('sources') to the first smoke test, it would make this dependency explicit and serve as a canary if the seed is ever removed.

Not a blocker, just a nice-to-have for future-proofing.

Nit: migration count comment in CLAUDE.md is stale

The CLAUDE.md says **Current migration count:** 84 (latest: \084_add_auto_favorite_targets`)` but this branch adds nothing new there, and the repo already has migrations up to 089. This is pre-existing and out of scope for this PR — noting it only so it gets updated in a future pass.

Verdict

Approve. The previous concern (redundant dedupe DDL) is cleanly resolved. The implementation is correct, the scope is appropriately narrow (part 1 as described), and the smoke test gives good confidence. The minor items above are all non-blocking observations.