Skip to content

feat: adopt node-pg-migrate for database migrations (#78)#88

Merged
BillChirico merged 10 commits intomainfrom
feat/database-migrations
Feb 26, 2026
Merged

feat: adopt node-pg-migrate for database migrations (#78)#88
BillChirico merged 10 commits intomainfrom
feat/database-migrations

Conversation

@BillChirico
Copy link
Collaborator

Summary

Replaces all inline DDL scattered across db.js, postgres.js, and restartTracker.js with proper versioned migrations via node-pg-migrate.

Closes #78

Changes

  • Migrations: Added node-pg-migrate + initial migration (001_initial-schema.cjs) capturing all 8 tables and all indexes
  • src/db.js: Removed ~200 lines of inline DDL from initDb(). Now: connect → test → run pending migrations → return pool
  • src/transports/postgres.js: Removed initLogsTable() — table created by migration
  • src/utils/restartTracker.js: Removed ensureTable() and self-healing DDL — table created by migration
  • src/index.js + src/config-listeners.js: Removed initLogsTable import/calls
  • Docker: Updated Dockerfile to copy migrations directory
  • Scripts: Added migrate, migrate:up, migrate:down, migrate:create to package.json

Key Decisions

  • .cjs migration files — node-pg-migrate uses exports.up/down which conflicts with ESM. CJS files work perfectly via require()
  • Programmatic runner — app self-migrates on startup via initDb(). CLI scripts available for manual use
  • Idempotent initial migrationCREATE TABLE IF NOT EXISTS throughout, safe on existing production databases
  • Down migration drops in reverse FK order

Files Changed

File Change
migrations/001_initial-schema.cjs New — all 8 tables + indexes
src/db.js Stripped inline DDL, added migration runner
src/transports/postgres.js Removed initLogsTable()
src/utils/restartTracker.js Removed ensureTable() + self-healing
src/index.js Removed initLogsTable import/call
src/config-listeners.js Removed initLogsTable import/call
package.json Added dep + scripts
Dockerfile Copy migrations dir

Testing

  • 70 test files, 1468 tests passing, 0 failures
  • Updated tests for db, postgres transport, restartTracker, index, config-listeners

Breaking Changes

None — existing databases migrate seamlessly. The initial migration is idempotent.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 25, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 04fb776 and e62e11b.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (13)
  • Dockerfile
  • migrations/001_initial-schema.cjs
  • package.json
  • src/config-listeners.js
  • src/db.js
  • src/index.js
  • src/transports/postgres.js
  • src/utils/restartTracker.js
  • tests/config-listeners.test.js
  • tests/db.test.js
  • tests/index.test.js
  • tests/transports/postgres.test.js
  • tests/utils/restartTracker.test.js

📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • Introduced a database migration system to manage schema initialization and updates in a version-controlled manner.
  • Improvements

    • Refactored database schema setup to use migrations instead of inline initialization for better maintainability.
    • Removed automatic table creation logic; all schema setup now handled through migrations.
  • Breaking Changes

    • Increased minimum Node.js version requirement to 20.11.0 (previously 18.0.0).

Walkthrough

Replaces inline DB schema setup with node-pg-migrate migrations. Adds an initial migration, migration runner in src/db.js, migration scripts and dependency, updates Dockerfile, removes runtime table-creation helpers (e.g., initLogsTable) and related self-healing logic.

Changes

Cohort / File(s) Summary
Migrations & manifest
migrations/001_initial-schema.cjs, Dockerfile, package.json
Adds initial idempotent migration creating eight tables and indexes; copies migrations/ into Docker image; adds node-pg-migrate dependency, migration scripts, and bumps Node engine to >=20.11.0.
DB init / migration runner
src/db.js
Removes inline DDL; adds runMigrations() and invokes node-pg-migrate (up) during initDb, preserving pool lifecycle and idempotency.
Logging transport & startup
src/transports/postgres.js, src/config-listeners.js, src/index.js
Removes exported initLogsTable and all runtime table-creation calls; startup/config-change now enables Postgres transport and calls pruneOldLogs without creating tables.
Restart tracking util
src/utils/restartTracker.js
Removes ensureTable and self-healing logic; recordRestart/queries no longer attempt to create the bot_restarts table on error.
Tests
tests/db.test.js, tests/index.test.js, tests/config-listeners.test.js, tests/transports/postgres.test.js, tests/utils/restartTracker.test.js
Updated mocks and assertions to expect migration runner invocation and removed initLogsTable-related tests and self-healing cases; adjusted DB query expectations to direct INSERTs.

Possibly related PRs

  • #88 — Mirrors these changes: adds migrations/, node-pg-migrate, updates Dockerfile and src/db.js to run migrations (strong overlap).
  • #18 — Introduced the conversations table via inline setup; this PR extracts that schema into the migration file.
  • #65 — Modified PostgreSQL logging initialization paths; related because this PR removes initLogsTable and adjusts logging startup flow.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat: adopt node-pg-migrate for database migrations (#78)' clearly and specifically summarizes the main change: adopting a migration framework.
Description check ✅ Passed The description comprehensively details the migration adoption, changes made to each file, key decisions, and testing status, all directly related to the changeset.
Linked Issues check ✅ Passed The PR fully implements all coding requirements from issue #78: adds node-pg-migrate, creates idempotent initial migration with all 8 tables and indexes, removes inline DDL from db.js and related modules, runs migrations programmatically in initDb(), adds migration scripts to package.json, and updates Dockerfile.
Out of Scope Changes check ✅ Passed All changes are directly related to adopting node-pg-migrate for migrations: migration framework setup, DDL extraction, removal of old table-creation patterns, and test updates to reflect the new migration-based approach.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/database-migrations

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 25, 2026

Note

Docstrings generation - SUCCESS
Generated docstrings and committed to branch feat/database-migrations (commit: 57fba79f14c9fae72e57fb5898bbaf8c2fb1142c)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@migrations/001_initial-schema.cjs`:
- Around line 103-105: The index idx_ai_usage_user_created on ai_usage(user_id,
created_at) will include many NULL user_id rows; replace it with a partial index
that only indexes rows where user_id IS NOT NULL to avoid index bloat. Locate
the CREATE INDEX statement for idx_ai_usage_user_created and change it so the
migration drops or does not create the full index and instead creates a partial
index on (user_id, created_at) with a WHERE clause user_id IS NOT NULL (use the
same index name or a new one as appropriate) so queries filtering by user_id use
a compact index.
- Around line 62-74: The foreign key on column case_id in the CREATE TABLE for
mod_scheduled_actions currently reads "case_id INTEGER REFERENCES mod_cases(id)"
which defaults to RESTRICT; update that reference to include an explicit ON
DELETE policy (e.g., "case_id INTEGER REFERENCES mod_cases(id) ON DELETE
CASCADE" to remove scheduled actions when a case is deleted, or "case_id INTEGER
REFERENCES mod_cases(id) ON DELETE SET NULL" and make case_id nullable if you
prefer to keep actions but disassociate them). Modify the CREATE TABLE SQL in
the migrations/001_initial-schema.cjs diff (the mod_scheduled_actions
definition) to include the chosen ON DELETE clause and adjust NULL/NOT NULL on
case_id as required.

In `@package.json`:
- Around line 18-19: The package.json defines two identical npm scripts,
"migrate" and "migrate:up", which is redundant; update package.json to remove
the duplicate by either deleting one of the entries (e.g., remove "migrate:up")
or make "migrate" a canonical alias with a clear comment and keep only one
implementation so that only a single script (either "migrate" or "migrate:up")
runs node-pg-migrate up --migrations-dir migrations to avoid duplication.
- Line 31: The package.json declares "node-pg-migrate" at ^8.0.4 which requires
Node >=20.11.0 but the "engines" field still allows "node": ">=18.0.0"; update
the package.json "engines" entry to require Node >=20.11.0 (e.g. "node":
">=20.11.0") and ensure any other engine declarations (the other engines entries
referenced around lines 43-45) are updated consistently; also verify CI/test
matrix and any documentation mentioning Node versions are aligned with this new
minimum.

In `@TASK.md`:
- Around line 119-129: Update the acceptance criterion that currently reads
"Initial migration captures all 9 tables + all indexes" to "Initial migration
captures all 8 tables + all indexes" to match the spec and migration; ensure the
eight table names (config, conversations, mod_cases, mod_scheduled_actions,
memory_optouts, ai_usage, logs, bot_restarts) are reflected implicitly by the
corrected count so the acceptance checklist matches the actual migration file
and Section 3 of the spec.
- Around line 7-9: Update the stale metadata in TASK.md by replacing the PR
field value from "TBD" to "#88" and change the State field from "🔄 In Progress"
to a completed status (e.g., "✅ Done" or "Completed") so the PR and state
reflect the finished work.
- Around line 3-9: The file has markdownlint violations MD022/MD031 due to
missing blank lines around headings and fenced code blocks (e.g., the "##
Status" heading and other headings/blocks); update TASK.md by inserting a single
blank line before and after each heading and fenced code block so that every
heading (like "## Status") is separated from adjacent content and all fenced
code blocks have blank lines above and below, then re-run markdownlint to ensure
MD022 and MD031 are resolved.

In `@tests/db.test.js`:
- Around line 103-118: The test currently asserts migration runner options but
omits verifying the log callback; update the test that inspects
migrationMocks.runner.mock.calls[0][0] (runnerOpts) to also assert that
runnerOpts.log is a function, ensuring the log integration passed from src/db.js
remains present (use migrationMocks.runner and runnerOpts.log to locate the
check).

ℹ️ Review info

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c3781ac and c219fb2.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (14)
  • Dockerfile
  • TASK.md
  • migrations/001_initial-schema.cjs
  • package.json
  • src/config-listeners.js
  • src/db.js
  • src/index.js
  • src/transports/postgres.js
  • src/utils/restartTracker.js
  • tests/config-listeners.test.js
  • tests/db.test.js
  • tests/index.test.js
  • tests/transports/postgres.test.js
  • tests/utils/restartTracker.test.js
💤 Files with no reviewable changes (4)
  • src/config-listeners.js
  • src/utils/restartTracker.js
  • src/transports/postgres.js
  • tests/index.test.js
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.js

📄 CodeRabbit inference engine (AGENTS.md)

**/*.js: Use ESM modules only — use import/export, never require()
Use node: protocol for Node.js builtins (e.g. import { readFileSync } from 'node:fs')
Always use semicolons
Use single quotes for strings
Use 2-space indentation
No TypeScript — use plain JavaScript with JSDoc comments for documentation

Files:

  • tests/config-listeners.test.js
  • tests/db.test.js
  • tests/transports/postgres.test.js
  • src/db.js
  • tests/utils/restartTracker.test.js
  • src/index.js
tests/**/*.js

📄 CodeRabbit inference engine (AGENTS.md)

Test files must achieve at least 80% code coverage on statements, branches, functions, and lines

Files:

  • tests/config-listeners.test.js
  • tests/db.test.js
  • tests/transports/postgres.test.js
  • tests/utils/restartTracker.test.js
src/**/*.js

📄 CodeRabbit inference engine (AGENTS.md)

src/**/*.js: Always use Winston for logging — import { info, warn, error } from ../logger.js
NEVER use console.log, console.warn, console.error, or any console.* method in src/ files
Pass structured metadata to Winston logging calls (e.g. info('Message processed', { userId, channelId }))
Use custom error classes from src/utils/errors.js for error handling
Always log errors with context before re-throwing
Use getConfig(guildId?) from src/modules/config.js to read config
Use setConfigValue(path, value, guildId?) from src/modules/config.js to update config at runtime
Use splitMessage() utility for messages exceeding Discord's 2000-character limit
Use safeSend() wrapper for outgoing Discord messages to sanitize mentions and enforce allowedMentions

Files:

  • src/db.js
  • src/index.js
🧠 Learnings (4)
📚 Learning: 2026-02-25T02:39:33.506Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T02:39:33.506Z
Learning: Stateful objects requiring runtime config changes must use reactive `onConfigChange` wiring in `src/index.js` startup

Applied to files:

  • tests/config-listeners.test.js
📚 Learning: 2026-02-25T02:39:08.568Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-25T02:39:08.568Z
Learning: After completing code changes, rebuild the Docker container using `docker compose build` but do not start it

Applied to files:

  • Dockerfile
📚 Learning: 2026-02-25T02:39:33.506Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T02:39:33.506Z
Learning: Applies to src/commands/*mod*.js : Moderation commands must follow the pattern: deferReply → validate inputs → sendDmNotification → execute Discord action → createCase → sendModLogEmbed → checkEscalation

Applied to files:

  • src/index.js
📚 Learning: 2026-02-25T02:39:33.506Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T02:39:33.506Z
Learning: Applies to src/commands/*.js : Use `parseDuration()` from `src/utils/duration.js` for duration-based commands (timeout, tempban, slowmode)

Applied to files:

  • src/index.js
🧬 Code graph analysis (3)
tests/config-listeners.test.js (1)
src/logger.js (1)
  • addPostgresTransport (252-267)
src/db.js (5)
src/index.js (2)
  • __filename (50-50)
  • __dirname (51-51)
src/logger.js (2)
  • __dirname (19-19)
  • info (223-225)
src/modules/config.js (2)
  • __dirname (14-14)
  • path (430-430)
src/commands/config.js (2)
  • path (108-108)
  • path (283-283)
src/modules/memory.js (1)
  • msg (72-72)
tests/utils/restartTracker.test.js (1)
src/utils/restartTracker.js (1)
  • recordRestart (25-41)
🪛 markdownlint-cli2 (0.21.0)
TASK.md

[warning] 3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 17-17: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 42-42: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 47-47: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 52-52: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 53-53: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


[warning] 57-57: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 60-60: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 77-77: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 84-84: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 89-89: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 94-94: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 95-95: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


[warning] 104-104: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 110-110: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 114-114: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 119-119: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 131-131: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 137-137: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

🔇 Additional comments (12)
tests/utils/restartTracker.test.js (1)

49-96: LGTM — test assertions correctly reflect the removed ensureTable indirection.

The assertion at Line 59 now validates the first query is the INSERT INTO bot_restarts directly (index [0]), and Line 80 does the same for the defaults test. This accurately mirrors the simplified production code where no preceding CREATE TABLE call exists.

Dockerfile (1)

23-27: LGTM — migrations directory is correctly copied into the production image.

This aligns with the programmatic migration runner in src/db.js which reads migration files at startup.

tests/transports/postgres.test.js (1)

1-3: LGTM — initLogsTable import and test suite correctly removed.

The test file now accurately mirrors the production src/transports/postgres.js surface area post-refactor.

src/index.js (1)

41-41: LGTM — initLogsTable import cleanly replaced with just pruneOldLogs.

The startup sequence in startup() (Lines 354–370) correctly wires up the transport via addPostgresTransport and prunes old logs, with schema creation now handled by the migration runner in initDb().

tests/config-listeners.test.js (1)

268-291: LGTM — error path correctly updated to simulate addPostgresTransport throwing.

addPostgresTransport is synchronous (per src/logger.js lines 251–266), so a synchronous .mockImplementationOnce(() => { throw ... }) is the correct mock strategy here.

migrations/001_initial-schema.cjs (1)

11-12: ⚠️ Potential issue | 🔴 Critical

Migrations must use ESM syntax for node-pg-migrate v8 compatibility.

The migration file migrations/001_initial-schema.cjs uses CommonJS syntax (exports.up/down) with a .cjs extension. However, node-pg-migrate v8 (ESM-only) expects migrations as .js files with ESM syntax (export const up/down). The .cjs approach is undocumented and unsupported; v8's CLI and documentation do not mention .cjs migration files at all, only js, ts, or sql.

Rename the file to migrations/001_initial-schema.js and convert to ESM:

/** `@param` {import('node-pg-migrate').MigrationBuilder} pgm */
export const up = (pgm) => {
  // migration code
};

export const down = (pgm) => {
  // rollback code
};
⛔ Skipped due to learnings
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T02:39:33.506Z
Learning: Applies to **/*.js : Use ESM modules only — use `import`/`export`, never `require()`
src/db.js (3)

6-10: LGTM — imports follow ESM and node: protocol conventions.

All new imports use ESM syntax and the node: prefix for built-in modules. Winston logger is correctly imported for use within src/.


106-125: Migration failure cleanup path is correct and well-structured.

The try/catch around both the connection test and migration run properly cleans up the pool on any failure (Line 122), preventing getPool() from returning a stale instance. The pool = null reset on Line 123 ensures subsequent initDb() calls can retry cleanly.


59-73: 🧹 Nitpick | 🔵 Trivial

Use the structured logger object pattern instead of log callback to differentiate log levels.

The databaseUrl and log options are correct per the node-pg-migrate programmatic API. However, using log: (msg) => info(msg) forwards all migration output (including verbose per-statement logs) at info level, which could be noisy in production. Instead, use the logger object pattern to separate verbose logs at debug level from important summary messages:

log: undefined,
logger: {
  debug: (msg) => debug(msg),
  info: (msg) => info(msg),
  warn: (msg) => warn(msg),
  error: (msg) => error(msg),
}

This allows per-migration logs at debug level and keeps the summary message on Line 72 at info level.

⛔ Skipped due to learnings
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T02:39:33.506Z
Learning: Applies to src/**/*.js : Always use Winston for logging — import `{ info, warn, error }` from `../logger.js`
tests/db.test.js (3)

13-20: Migration mock setup is clean and correctly hoisted.

The vi.hoisted pattern ensures the mock is available before module loading, and the mock correctly mirrors the named export structure of node-pg-migrate.


163-168: Migration failure cleanup test is well-structured and mirrors the connection failure test.

This correctly verifies that when runner rejects, the pool is ended and the error propagates. Good addition that covers the new failure path introduced by the migration step.


120-127: Idempotency test correctly validates single migration run.

Asserting migrationMocks.runner is called exactly once across two initDb() calls confirms the pool-caching logic prevents redundant migration attempts.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/db.js (1)

124-129: ⚠️ Potential issue | 🟡 Minor

Log context before rethrowing DB init failures.

This catch cleans up and rethrows but does not emit contextual error telemetry for whether the failure happened during connection test vs migration run. Please log before rethrowing.

Suggested patch
     } catch (err) {
+      logError('Database initialization failed', {
+        stage: 'connection-test-or-migrations',
+        error: err.message,
+      });
       // Clean up the pool so getPool() doesn't return an unusable instance
       await pool.end().catch(() => {});
       pool = null;
       throw err;
     }

As per coding guidelines: "Always log errors with context before re-throwing."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/db.js` around lines 124 - 129, The catch block that cleans up the pool
and rethrows (around the pool.end() call and variable pool) must log the error
with context before rethrowing; capture the thrown err and call the existing
telemetry/logger (or console.error) with a message that includes the error and
which phase failed (e.g., "connection test" vs "migration run") — implement this
by adding a local phase variable (set to "connection test" before the connection
check and "migration" before running migrations) and logging `phase` plus `err`
right before `await pool.end().catch(() => {}); pool = null; throw err;` so the
log shows whether the failure occurred during the connection test or migration
step.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/db.js`:
- Around line 64-67: runMigrations currently passes a raw connection string to
node-pg-migrate's runner(), which ignores the SSL options used by the app pool;
change runMigrations to pass a pg Client config object (e.g., {
connectionString: databaseUrl, ssl }) so runner() receives the same SSL config
returned by getSslConfig(connectionString), and update the call site to pass
getSslConfig(connectionString) along with connectionString; also enhance the
catch in initialize (or the function that calls runMigrations) to log a
contextual error message (including err.message) before cleaning up the pool and
rethrowing to preserve diagnostics (reference symbols: runMigrations, runner,
getSslConfig, connectionString, pool).

In `@src/utils/restartTracker.js`:
- Around line 21-24: Update the JSDoc for the async functions recordRestart and
getRestarts to include the missing pool parameter and reflect async return
types: add a `@param` {Pool} pool (or appropriate pool type) description to both
docblocks and change the `@returns` to Promise<number|null> for recordRestart and
Promise<Array|whatever the actual type> or
Promise<RestartRow[]>/Promise<Array<Object>> for getRestarts (use the existing
restart row shape), ensuring the descriptions match their runtime behavior and
keep plain-JS JSDoc style.

---

Outside diff comments:
In `@src/db.js`:
- Around line 124-129: The catch block that cleans up the pool and rethrows
(around the pool.end() call and variable pool) must log the error with context
before rethrowing; capture the thrown err and call the existing telemetry/logger
(or console.error) with a message that includes the error and which phase failed
(e.g., "connection test" vs "migration run") — implement this by adding a local
phase variable (set to "connection test" before the connection check and
"migration" before running migrations) and logging `phase` plus `err` right
before `await pool.end().catch(() => {}); pool = null; throw err;` so the log
shows whether the failure occurred during the connection test or migration step.

ℹ️ Review info

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c219fb2 and 57fba79.

📒 Files selected for processing (4)
  • src/config-listeners.js
  • src/db.js
  • src/index.js
  • src/utils/restartTracker.js
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.js

📄 CodeRabbit inference engine (AGENTS.md)

**/*.js: Use ESM modules only — use import/export, never require()
Use node: protocol for Node.js builtins (e.g. import { readFileSync } from 'node:fs')
Always use semicolons
Use single quotes for strings
Use 2-space indentation
No TypeScript — use plain JavaScript with JSDoc comments for documentation

Files:

  • src/config-listeners.js
  • src/utils/restartTracker.js
  • src/index.js
  • src/db.js
src/**/*.js

📄 CodeRabbit inference engine (AGENTS.md)

src/**/*.js: Always use Winston for logging — import { info, warn, error } from ../logger.js
NEVER use console.log, console.warn, console.error, or any console.* method in src/ files
Pass structured metadata to Winston logging calls (e.g. info('Message processed', { userId, channelId }))
Use custom error classes from src/utils/errors.js for error handling
Always log errors with context before re-throwing
Use getConfig(guildId?) from src/modules/config.js to read config
Use setConfigValue(path, value, guildId?) from src/modules/config.js to update config at runtime
Use splitMessage() utility for messages exceeding Discord's 2000-character limit
Use safeSend() wrapper for outgoing Discord messages to sanitize mentions and enforce allowedMentions

Files:

  • src/config-listeners.js
  • src/utils/restartTracker.js
  • src/index.js
  • src/db.js
🧠 Learnings (3)
📚 Learning: 2026-02-25T02:39:33.506Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T02:39:33.506Z
Learning: Applies to src/**/*.js : Always use Winston for logging — import `{ info, warn, error }` from `../logger.js`

Applied to files:

  • src/config-listeners.js
📚 Learning: 2026-02-25T02:39:33.506Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T02:39:33.506Z
Learning: Applies to src/commands/*mod*.js : Moderation commands must follow the pattern: deferReply → validate inputs → sendDmNotification → execute Discord action → createCase → sendModLogEmbed → checkEscalation

Applied to files:

  • src/index.js
📚 Learning: 2026-02-25T02:39:33.506Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T02:39:33.506Z
Learning: Applies to src/commands/*.js : Use `parseDuration()` from `src/utils/duration.js` for duration-based commands (timeout, tempban, slowmode)

Applied to files:

  • src/index.js
🧬 Code graph analysis (1)
src/db.js (3)
src/index.js (2)
  • __filename (50-50)
  • __dirname (51-51)
src/logger.js (2)
  • __dirname (19-19)
  • info (223-225)
src/modules/config.js (1)
  • __dirname (14-14)
🔇 Additional comments (2)
src/config-listeners.js (1)

33-38: JSDoc improvement is clear and accurate.

This doc block correctly captures the enable/recreate/disable behavior and the changePath logging context.

src/index.js (1)

41-41: Startup flow cleanup looks correct for migration-first schema management.

Dropping runtime table-init coupling and documenting prune-on-startup behavior is consistent with the new migration-driven DB initialization.

Also applies to: 316-319

coderabbitai[bot]
coderabbitai bot previously approved these changes Feb 25, 2026
Captures all 8 tables and indexes from existing inline DDL:
config, conversations, mod_cases, mod_scheduled_actions,
memory_optouts, ai_usage, logs, bot_restarts.

Uses IF NOT EXISTS throughout for idempotency on existing DBs.
Down migration drops in reverse FK order.
- src/db.js: replaced all CREATE TABLE/INDEX/ALTER TABLE with
  programmatic node-pg-migrate runner call
- src/transports/postgres.js: removed initLogsTable()
- src/utils/restartTracker.js: removed ensureTable() and
  self-healing 42P01 catch block
- src/index.js: removed initLogsTable() call
- src/config-listeners.js: removed initLogsTable() call
- package.json: migrate, migrate:up, migrate:down, migrate:create
- Dockerfile: copy migrations/ directory into image
- db.test.js: mock node-pg-migrate runner, test migration runs
  on init, test migration failure cleanup
- transports/postgres.test.js: remove initLogsTable tests (function removed)
- utils/restartTracker.test.js: remove ensureTable and self-healing tests
- index.test.js: remove initLogsTable mock and assertions
- config-listeners.test.js: remove initLogsTable references
@BillChirico BillChirico force-pushed the feat/database-migrations branch from 58ff5dc to e62e11b Compare February 26, 2026 02:14
@BillChirico BillChirico merged commit 3c9401d into main Feb 26, 2026
0 of 4 checks passed
@BillChirico BillChirico deleted the feat/database-migrations branch February 26, 2026 02:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adopt node-pg-migrate for database migrations

1 participant