Claude finished @Yeraze's task —— View job

PR Review: refactor(test): migrate messages repo tests to createTestDb (section B2, part 2)

Tasks

• Gather context and understand the request
• Read changed test files
• Read createTestDb helper
• Analyze code quality, bugs, performance, security, test coverage
• Post review feedback

Summary

This is a clean, well-executed refactor. The migration from hand-rolled DDL to createTestDb() is the right approach, and the net -194 lines is a meaningful reduction in maintenance surface. All five files follow the same consistent pattern. A few observations:

Bug: Incomplete node seed in messages.purge.test.ts:131

The broadcast-node insert at line 131 is missing the three NOT NULL columns that the rest of this file correctly supplies:

// Current (line 131) — missing sourceId, createdAt, updatedAt
db.exec(`INSERT OR IGNORE INTO nodes (nodeNum, nodeId) VALUES (${0xffffffff}, '!ffffffff')`);

// Should match the pattern used everywhere else in this file
const now = Date.now();
db.prepare(
  "INSERT OR IGNORE INTO nodes (nodeNum, nodeId, sourceId, createdAt, updatedAt) VALUES (?, ?, 'default', ?, ?)"
).run(0xffffffff, '!ffffffff', now, now);

SQLite's INSERT OR IGNORE will silently swallow the NOT NULL constraint violation (it treats it as a conflict), so the node row never gets inserted. The test then passes only because the FK enforcement may also be absent in this in-memory setup — meaning the test passes for the wrong reason. This is the exact class of drift the B2 migration is designed to prevent.

Minor: db.exec() vs db.prepare() inconsistency

Relatedly, the same line uses db.exec() with string interpolation (${0xffffffff}) while every other seed in all five files uses db.prepare() with parameterized runs. The interpolation is safe here (it's a numeric literal, not user input), but the inconsistency is worth flagging for style uniformity.

Positive observations

• Pattern consistency across all five files is excellent. Every file follows the same createTestDb() → seed nodes → construct repo structure.
• Comment quality is high. The notes about "full schema NOT NULL/PK columns: sourceId, timestamps" directly explain the friction encountered.
• afterEach(() => db.close()) is correctly present in all five files — no resource leak risk.
• messages.createdAt-ordering.test.ts covers the cursor-based pagination case (getMessagesBeforeInChannel) which is the subtlest ordering edge case; good to see it explicitly tested.
• messages.exclude-portnums.test.ts correctly uses rxTime as createdAt in the insert helper (with the inline comment explaining why), showing awareness of the [BUG] Large Meshtastic TCP node repeatedly disconnects during/after full config sync; passive per-source mode improves stability #3122 ordering change.
• The createTestDb() helper itself (testDb.ts) is well-designed: the getSetting/setSetting shims that bridge the pre-settings-table window are a nice defensive touch.

Suggestion: Use close() from TestDb

createTestDb() returns a close() method on the TestDb interface for exactly this purpose. All five tests call db.close() directly on the raw sqlite handle instead of t.close(). Both are equivalent today, but using the interface method is more future-proof if close() ever needs to do cleanup beyond closing the connection:

// Current
afterEach(() => { db.close(); });

// More idiomatic given the TestDb interface
afterEach(() => { close(); }); // destructure close from createTestDb()
// or:
afterEach(() => { testDb.close(); });

This is stylistic, not a correctness issue.

The broadcast-node seed bug is the only substantive issue. The rest is a clean, well-reasoned migration. The B2 approach of small, verified batches is the right strategy for this kind of refactor.

Claude finished @Yeraze's task —— View job

PR Review: refactor(test): migrate messages repo tests to createTestDb (section B2, part 2)

Tasks

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

Summary

This is a clean, well-executed refactor. The migration from hand-rolled DDL to createTestDb() is the right approach, and the net −194 lines is a meaningful reduction in maintenance surface. All five files follow the same consistent pattern.

Previously identified bug: Fixed ✅

The broadcast-node seed in messages.purge.test.ts:131 that was flagged in the earlier review has been corrected. The fix is correct and complete — the node insert at lines 134–136 now supplies all required columns (sourceId, createdAt, updatedAt):

// Fixed — all NOT NULL/PK columns present
db.prepare(
  "INSERT OR IGNORE INTO nodes (nodeNum, nodeId, sourceId, createdAt, updatedAt) VALUES (?, ?, 'default', ?, ?)",
).run(0xffffffff, '!ffffffff', now, now);

The inline comment at line 131–132 also documents exactly why the full column set is needed, which is a good addition for future readers.

Remaining observations (minor/stylistic)

db.close() vs testDb.close() — All five files call db.close() on the raw sqlite handle in afterEach rather than t.close() (the TestDb interface method). Both are equivalent today, but using the interface method is marginally more future-proof if cleanup ever needs to expand. This is stylistic, not a correctness issue.

Test coverage quality — All five files cover the right edge cases:

• messages.purge: source isolation, broadcast exclusion, undefined-source wildcard purge, regression for [BUG] Error fetching telemetry: SqliteError: no such column: source_id #2631
• messages.insert: duplicate detection (three angles: return value, idempotency, row count)
• messages.timestamp: null rxTime, wrong requestId, adjacent-row isolation
• messages.createdAt-ordering: future-skewed clock, cursor-based pagination (getMessagesBeforeInChannel), sync variants
• messages.exclude-portnums: null portnum (legacy data), DM eviction bug ([BUG] Successfull traceroute with node makes last message in chat with that node dissapeared #2741)

No gaps found. The regression comments linking issue numbers (#2631, #2741, #3122) are particularly helpful.

Positive observations

• Pattern consistency across all five files is excellent — same createTestDb() → seed nodes → construct repo → afterEach close structure throughout.
• Comment quality is high. The "full schema NOT NULL/PK columns" notes in every file directly explain the migration friction.
• afterEach(() => db.close()) is correctly present in all five files with no resource leaks.
• The incremental B2 strategy (small, verified batches with confirmed test counts) is the right approach for this kind of large refactor.

Verdict

Clean PR. The one substantive bug from the previous review has been addressed. No new issues found.