Conversation
eduardoboucas
commented
Apr 20, 2026
eduardoboucas
commented
📝 WalkthroughSummary by CodeRabbitNew Features
Tests
WalkthroughThis pull request introduces database migration status monitoring functionality for Netlify databases. It adds a new module defining shared migration file types and two Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/commands/database/db-connection.ts (1)
72-89:⚠️ Potential issue | 🟠 MajorStop
NetlifyDevwhen the pg client fails to connect.After
await netlifyDev.start()on line 72, a rejection fromclient.connect()on line 81 exits this function before the cleanup handler is ever registered, leaving the spawned dev process running. Additionally, ifclient.end()throws within cleanup,netlifyDev.stop()is never called.Wrap
client.connect()in a try-catch to stopNetlifyDevon failure, and wrap the cleanup operations in try-finally to ensureNetlifyDevis always stopped:Suggested fix
const client = new Client({ connectionString }) - await client.connect() + try { + await client.connect() + } catch (error) { + await netlifyDev.stop() + throw error + } return { client, connectionString, cleanup: async () => { - await client.end() - await netlifyDev.stop() + try { + await client.end() + } finally { + await netlifyDev.stop() + } }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/database/db-connection.ts` around lines 72 - 89, After starting NetlifyDev with netlifyDev.start(), ensure netlifyDev.stop() is always called if client.connect() fails by wrapping the connection in a try/catch that calls await netlifyDev.stop() before rethrowing; locate the Client instantiation and the await client.connect() call and add the try/catch around it. Also make the returned cleanup ensure netlifyDev.stop() runs even if client.end() throws by changing cleanup to use try/finally: attempt await client.end() in the try and always await netlifyDev.stop() in the finally block so the dev process is always stopped.
🧹 Nitpick comments (1)
src/commands/database/status-db.ts (1)
52-59: Share the migration version parser.This parser is duplicated verbatim in
src/commands/database/applied-migrations.ts. If the naming rules change in one place, disk parsing and applied-migration parsing will drift andcomputeStatus()will start classifying the same migration differently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/database/status-db.ts` around lines 52 - 59, The parseVersion logic is duplicated (function parseVersion) and must be centralized: extract the parser into a single shared utility (e.g., export a parseMigrationVersion function from a new or existing util module) and replace the inline parseVersion definitions in both src/commands/database/status-db.ts and src/commands/database/applied-migrations.ts with imports of that shared function; ensure the exported name matches usages and keep the same behavior and return type (number | null) so computeStatus() and applied-migration parsing stay consistent.
🤖 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/commands/database/status-db.ts`:
- Around line 323-335: The message shown when status.outOfOrder has entries is
incorrect: replace the current remediation text that suggests running
formatCommand('db migrations reset') with guidance to remove or renumber the
local migration files and re-generate them with a higher prefix (e.g.
delete/rename the local migration directories listed in status.outOfOrder and
run the migration generation command again). Also gate any suggestion to run
formatCommand('db reset') so it’s only displayed for local-only flows (detect
via the existing status metadata that indicates a remote baseline or local-only
state) rather than as a general fix. Ensure you update the strings produced
where status.outOfOrder is formatted and keep references to the offending
migrations (status.outOfOrder.map(...)) so users can identify which local files
to delete/regenerate.
- Around line 270-284: The output mistakenly suggests running `db connect`/`db
connect --query` for branch status even though connect.ts
(connectRawClient/buildDir) connects to the local DB; wrap the call to
logConnectCommands and the follow-up secondary messages in a guard that only
shows those connect instructions when the status is for the local DB (not when
invoked in branch/--branch mode or when the connectionString clearly refers to a
remote branch). Update the logic around connectionString, showCredentials,
logConnectCommands, connectionStringHasCredentials, primary and secondary so
that when a branch flag (or a predicate like isBranchTarget(connectionString))
is true you omit logConnectCommands and instead keep only the safe informational
message about the branch connection string. Ensure the new predicate is used
where connectionString is checked so suggestions don't point to the wrong
target.
---
Outside diff comments:
In `@src/commands/database/db-connection.ts`:
- Around line 72-89: After starting NetlifyDev with netlifyDev.start(), ensure
netlifyDev.stop() is always called if client.connect() fails by wrapping the
connection in a try/catch that calls await netlifyDev.stop() before rethrowing;
locate the Client instantiation and the await client.connect() call and add the
try/catch around it. Also make the returned cleanup ensure netlifyDev.stop()
runs even if client.end() throws by changing cleanup to use try/finally: attempt
await client.end() in the try and always await netlifyDev.stop() in the finally
block so the dev process is always stopped.
---
Nitpick comments:
In `@src/commands/database/status-db.ts`:
- Around line 52-59: The parseVersion logic is duplicated (function
parseVersion) and must be centralized: extract the parser into a single shared
utility (e.g., export a parseMigrationVersion function from a new or existing
util module) and replace the inline parseVersion definitions in both
src/commands/database/status-db.ts and
src/commands/database/applied-migrations.ts with imports of that shared
function; ensure the exported name matches usages and keep the same behavior and
return type (number | null) so computeStatus() and applied-migration parsing
stay consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7fd9b746-877b-488b-8db0-be9172a64116
📒 Files selected for processing (6)
src/commands/database/applied-migrations.tssrc/commands/database/constants.tssrc/commands/database/database.tssrc/commands/database/db-connection.tssrc/commands/database/status-db.tstests/unit/commands/database/status-db.test.ts
| if (connectionString) { | ||
| const displayed = showCredentials ? connectionString : redactConnectionString(connectionString) | ||
|
|
||
| primary(STATUS_GOOD, `Connected to database branch: ${displayed}`) | ||
| logConnectCommands() | ||
|
|
||
| if (!showCredentials && connectionStringHasCredentials(connectionString)) { | ||
| secondary( | ||
| `To reveal the full connection string (including credentials), run ${formatCommand( | ||
| 'db status --show-credentials', | ||
| )}`, | ||
| ) | ||
| } else { | ||
| secondary(`To connect to the database directly, use the connection string: ${displayed}`) | ||
| } |
There was a problem hiding this comment.
Don't suggest db connect for branch status output.
This block always prints db connect/db connect --query when a connectionString exists, but src/commands/database/connect.ts still calls connectRawClient(buildDir) with no branch override, so those commands connect to the local database, not the branch being reported here. In --branch mode that's misleading and can send follow-up queries to the wrong target.
Suggested fix
if (connectionString) {
const displayed = showCredentials ? connectionString : redactConnectionString(connectionString)
primary(STATUS_GOOD, `Connected to database branch: ${displayed}`)
- logConnectCommands()
+ if (isLocal) {
+ logConnectCommands()
+ }
if (!showCredentials && connectionStringHasCredentials(connectionString)) {
secondary(
`To reveal the full connection string (including credentials), run ${formatCommand(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/database/status-db.ts` around lines 270 - 284, The output
mistakenly suggests running `db connect`/`db connect --query` for branch status
even though connect.ts (connectRawClient/buildDir) connects to the local DB;
wrap the call to logConnectCommands and the follow-up secondary messages in a
guard that only shows those connect instructions when the status is for the
local DB (not when invoked in branch/--branch mode or when the connectionString
clearly refers to a remote branch). Update the logic around connectionString,
showCredentials, logConnectCommands, connectionStringHasCredentials, primary and
secondary so that when a branch flag (or a predicate like
isBranchTarget(connectionString)) is true you omit logConnectCommands and
instead keep only the safe informational message about the branch connection
string. Ensure the new predicate is used where connectionString is checked so
suggestions don't point to the wrong target.
| if (status.outOfOrder.length > 0) { | ||
| log( | ||
| ` Out of order: ${status.outOfOrder | ||
| .map((m) => chalk.red(`${m.name} (version ${String(m.version)} <= max applied ${String(m.maxApplied)})`)) | ||
| .join(', ')}`, | ||
| ) | ||
| log('') | ||
| log( | ||
| chalk.gray( | ||
| `Run ${formatCommand( | ||
| 'db migrations reset', | ||
| )} to delete these local-only migrations, then generate them again with a higher prefix.`, | ||
| ), |
There was a problem hiding this comment.
The out-of-order remediation points to the wrong fix.
db migrations reset is not a registered command, and even db reset only rebuilds the local dev database—it does not delete or renumber the migration directories that make this status out of order, especially in --branch mode where the comparison baseline is remote. The guidance here should focus on deleting/regenerating the local migration files, with any reset hint gated to local-only flows.
Suggested fix
if (status.outOfOrder.length > 0) {
log(
` Out of order: ${status.outOfOrder
.map((m) => chalk.red(`${m.name} (version ${String(m.version)} <= max applied ${String(m.maxApplied)})`))
.join(', ')}`,
)
log('')
log(
chalk.gray(
- `Run ${formatCommand(
- 'db migrations reset',
- )} to delete these local-only migrations, then generate them again with a higher prefix.`,
+ 'Delete or rename these local migration directories, then generate them again with a higher prefix.',
),
)
+ if (isLocal) {
+ log(chalk.gray(`If you need to rebuild the local dev database from scratch, run ${formatCommand('db reset')}.`))
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/database/status-db.ts` around lines 323 - 335, The message shown
when status.outOfOrder has entries is incorrect: replace the current remediation
text that suggests running formatCommand('db migrations reset') with guidance to
remove or renumber the local migration files and re-generate them with a higher
prefix (e.g. delete/rename the local migration directories listed in
status.outOfOrder and run the migration generation command again). Also gate any
suggestion to run formatCommand('db reset') so it’s only displayed for
local-only flows (detect via the existing status metadata that indicates a
remote baseline or local-only state) rather than as a general fix. Ensure you
update the strings produced where status.outOfOrder is formatted and keep
references to the offending migrations (status.outOfOrder.map(...)) so users can
identify which local files to delete/regenerate.
🤖 I have created a release *beep* *boop* --- ## [25.1.0](v25.0.1...v25.1.0) (2026-04-21) ### Features * add `db migrations reset` command ([#8177](#8177)) ([3dd0f38](3dd0f38)) * add `db status` command ([#8173](#8173)) ([9bccaf9](9bccaf9)) * Add deploy_source to CLI deploy requests (EX-2032) ([#8155](#8155)) ([289933d](289933d)) * restructure db command files ([#8175](#8175)) ([794c2e0](794c2e0)) * support `NETLIFY_DB_BRANCH` env var in `db status` command ([#8174](#8174)) ([5647420](5647420)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: token-generator-app[bot] <82042599+token-generator-app[bot]@users.noreply.github.com>
2 participants
