Merged
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds an experimental Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Key observations
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
📊 Benchmark resultsComparing with 42bac16
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/commands/database/connect.ts (2)
73-75: Guard against double cleanup invocation.
rl.on('close')triggershandleCleanup, which callsrl.close()again. While readline tolerates this, consider adding a guard to prevent redundant cleanup work.Proposed fix
+ let cleanedUp = false const handleCleanup = async () => { + if (cleanedUp) return + cleanedUp = true rl.close() await cleanup() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/database/connect.ts` around lines 73 - 75, The readline 'close' handler may call handleCleanup which itself calls rl.close(), risking redundant cleanup; add a guard (e.g., a boolean flag like isCleaningUp) scoped near the rl/handleCleanup declarations and check/set it at the start of handleCleanup and in the rl.on('close') callback so handleCleanup is a no-op if already running; reference rl, rl.on('close'), handleCleanup and rl.close() when adding the isCleaningUp guard and early-return logic.
60-71: SIGINT listener is never removed, causing potential handler accumulation.If
connectis invoked multiple times within the same process (e.g., in tests or a long-running CLI daemon), theSIGINThandler accumulates. Remove the listener on cleanup.Proposed fix
+ const sigintHandler = () => { + if (buffer) { + buffer = '' + process.stdout.write('\n') + rl.setPrompt('netlifydb=> ') + rl.prompt() + } else { + log('') + void handleCleanup() + } + } + - process.on('SIGINT', () => { - if (buffer) { - // Cancel current multi-line input - buffer = '' - process.stdout.write('\n') - rl.setPrompt('netlifydb=> ') - rl.prompt() - } else { - log('') - void handleCleanup() - } - }) + process.on('SIGINT', sigintHandler) const handleCleanup = async () => { + process.removeListener('SIGINT', sigintHandler) rl.close() await cleanup() }Note: Move
sigintHandlerdefinition beforehandleCleanupor extract the handler to allow removal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/database/connect.ts` around lines 60 - 71, The SIGINT handler registered with process.on('SIGINT', ...) in connect is never removed, causing accumulation; extract that anonymous handler into a named function (e.g., sigintHandler) that uses buffer, rl, and calls handleCleanup(), register it with process.on('SIGINT', sigintHandler), and ensure handleCleanup calls process.removeListener('SIGINT', sigintHandler) (or process.off) during cleanup so the listener is removed when the session ends.
🤖 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/connect.ts`:
- Line 38: The call to formatQueryResult is passing result.rows with an unsafe
type; cast result.rows to any[] to satisfy ESLint (same fix used in
meta-commands.ts). Update both occurrences where log(formatQueryResult(...
result.rows ...)) appears in connect.ts by changing the argument to result.rows
as any[] so the call becomes log(formatQueryResult(result.fields, result.rows as
any[], result.rowCount, result.command)), keeping the rest of the call intact.
In `@src/commands/database/database.ts`:
- Around line 93-107: Prettier formatting failed for the database CLI command;
run the formatter (or apply Prettier) on the file containing
dbCommand.command('connect') (the .action that imports ./connect.js and the
.addExamples block) to fix spacing/linebreaks and ensure the file matches
project style (e.g., run npx prettier --write src/commands/database/database.ts
or your IDE formatter), then re-stage the updated file so the CI Prettier check
passes.
In `@src/commands/database/meta-commands.ts`:
- Line 40: The ESLint failures come from assigning pg's any[] to typed outputs;
update every occurrence where result.rows is assigned (e.g., the return object
with rows: result.rows and the other result.rows usages in this file) to cast it
to the expected type like result.rows as Record<string, unknown>[] so the
returned/assigned rows are strongly typed; ensure you apply this same cast in
all places that reference result.rows in src/commands/database/meta-commands.ts
(including the instances around the existing return and the other usages
mentioned).
---
Nitpick comments:
In `@src/commands/database/connect.ts`:
- Around line 73-75: The readline 'close' handler may call handleCleanup which
itself calls rl.close(), risking redundant cleanup; add a guard (e.g., a boolean
flag like isCleaningUp) scoped near the rl/handleCleanup declarations and
check/set it at the start of handleCleanup and in the rl.on('close') callback so
handleCleanup is a no-op if already running; reference rl, rl.on('close'),
handleCleanup and rl.close() when adding the isCleaningUp guard and early-return
logic.
- Around line 60-71: The SIGINT handler registered with process.on('SIGINT',
...) in connect is never removed, causing accumulation; extract that anonymous
handler into a named function (e.g., sigintHandler) that uses buffer, rl, and
calls handleCleanup(), register it with process.on('SIGINT', sigintHandler), and
ensure handleCleanup calls process.removeListener('SIGINT', sigintHandler) (or
process.off) during cleanup so the listener is removed when the session ends.
🪄 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: 8ed3ab29-e950-40c4-b966-2de60a1c1c8c
📒 Files selected for processing (5)
src/commands/database/connect.tssrc/commands/database/database.tssrc/commands/database/db-connection.tssrc/commands/database/meta-commands.tssrc/commands/database/psql-formatter.ts
| if (options.json) { | ||
| logJson(result.rows) | ||
| } else { | ||
| log(formatQueryResult(result.fields, result.rows, result.rowCount, result.command)) |
There was a problem hiding this comment.
Fix unsafe any[] argument to satisfy ESLint.
Same issue as in meta-commands.ts—cast result.rows to fix the pipeline failure:
Proposed fix
- log(formatQueryResult(result.fields, result.rows, result.rowCount, result.command))
+ log(formatQueryResult(result.fields, result.rows as Record<string, unknown>[], result.rowCount, result.command))Apply to both line 38 and line 118.
Also applies to: 118-118
🧰 Tools
🪛 GitHub Actions: Lint
[error] 38-38: ESLint (@typescript-eslint/no-unsafe-argument): Unsafe argument of type any[] assigned to a parameter of type Record<string, unknown>[]
🪛 GitHub Check: Lint
[failure] 38-38:
Unsafe argument of type any[] assigned to a parameter of type Record<string, unknown>[]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/database/connect.ts` at line 38, The call to formatQueryResult
is passing result.rows with an unsafe type; cast result.rows to any[] to satisfy
ESLint (same fix used in meta-commands.ts). Update both occurrences where
log(formatQueryResult(... result.rows ...)) appears in connect.ts by changing
the argument to result.rows as any[] so the call becomes
log(formatQueryResult(result.fields, result.rows as any[], result.rowCount,
result.command)), keeping the rest of the call intact.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/commands/database/connect.ts (1)
22-23: Remove explanatory inline comments and rely on naming/structure.These comments describe behavior rather than intent. Please drop them and keep logic self-explanatory through small helper names if needed.
As per coding guidelines, "Never write comments on what the code does; make the code clean and self-explanatory instead".
Also applies to: 31-31, 46-46, 62-62, 78-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/database/connect.ts` around lines 22 - 23, Remove the explanatory inline comments that describe behavior (e.g., the comment above the conditional "if (options.json && !options.query)" and the other similar comments nearby) and make the code self-explanatory instead by using clear naming or small helpers; for example replace the raw condition with a well-named boolean or function (e.g., shouldPrintJsonConnection or isJsonModeWithoutQuery) and use that symbol in the if-statement so the intent is obvious without comments, then delete the redundant inline comment lines.
🤖 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/connect.ts`:
- Around line 29-36: The plain-text connection log (`log(\`Connected to
${connectionString}\`)`) is emitted before JSON one-shot output, breaking
`--json --query` consumers; suppress or defer that log when running one-shot
JSON mode by checking `options.query` && `options.json`, e.g., only call
`log(...)` if not both true, or move the `log(...)` after the `if
(options.query)` branch so `logJson(result.rows)` is the first/only output when
`options.query` and `options.json` are set; adjust the code paths around
`options.query`, `options.json`, `log()`, `logJson()` and the
`client.query(...)` call to ensure strictly JSON output in that case.
- Around line 55-75: The cleanup flow runs twice because handleCleanup() calls
rl.close(), which emits 'close' and triggers the rl.on('close') handler to call
handleCleanup() again; fix by making cleanup idempotent or by preventing the
duplicate invocation: add a module-level boolean flag (e.g., cleanedUp) that
handleCleanup(), the SIGINT handler, and the rl.on('close') handler check and
set so cleanup logic (calling cleanup(), netlifyDev.stop(), client.end(), etc.)
executes only once, and remove the call to rl.close() from handleCleanup() or
call rl.close() only when cleanedUp is false; reference functions/variables:
handleCleanup, rl.close, process.on('SIGINT'), rl.on('close'), cleanup,
netlifyDev.stop.
---
Nitpick comments:
In `@src/commands/database/connect.ts`:
- Around line 22-23: Remove the explanatory inline comments that describe
behavior (e.g., the comment above the conditional "if (options.json &&
!options.query)" and the other similar comments nearby) and make the code
self-explanatory instead by using clear naming or small helpers; for example
replace the raw condition with a well-named boolean or function (e.g.,
shouldPrintJsonConnection or isJsonModeWithoutQuery) and use that symbol in the
if-statement so the intent is obvious without comments, then delete the
redundant inline comment lines.
🪄 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: db75f229-10f0-4784-9562-39305702bba3
📒 Files selected for processing (4)
src/commands/database/connect.tssrc/commands/database/database.tssrc/commands/database/meta-commands.tssrc/commands/database/psql-formatter.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/commands/database/database.ts
- src/commands/database/psql-formatter.ts
- src/commands/database/meta-commands.ts
Comment on lines
+29
to
+36
| log(`Connected to ${connectionString}`) | ||
|
|
||
| // --query: one-shot mode | ||
| if (options.query) { | ||
| try { | ||
| const result = await client.query<Record<string, unknown>>(options.query) | ||
| if (options.json) { | ||
| logJson(result.rows) |
There was a problem hiding this comment.
Keep --json --query output strictly JSON.
At Line 29, plain-text logging runs before JSON output, so --json --query produces mixed output. That breaks JSON consumers.
Proposed fix
- log(`Connected to ${connectionString}`)
+ if (!options.json) {
+ log(`Connected to ${connectionString}`)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| log(`Connected to ${connectionString}`) | |
| // --query: one-shot mode | |
| if (options.query) { | |
| try { | |
| const result = await client.query<Record<string, unknown>>(options.query) | |
| if (options.json) { | |
| logJson(result.rows) | |
| if (!options.json) { | |
| log(`Connected to ${connectionString}`) | |
| } | |
| // --query: one-shot mode | |
| if (options.query) { | |
| try { | |
| const result = await client.query<Record<string, unknown>>(options.query) | |
| if (options.json) { | |
| logJson(result.rows) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/database/connect.ts` around lines 29 - 36, The plain-text
connection log (`log(\`Connected to ${connectionString}\`)`) is emitted before
JSON one-shot output, breaking `--json --query` consumers; suppress or defer
that log when running one-shot JSON mode by checking `options.query` &&
`options.json`, e.g., only call `log(...)` if not both true, or move the
`log(...)` after the `if (options.query)` branch so `logJson(result.rows)` is
the first/only output when `options.query` and `options.json` are set; adjust
the code paths around `options.query`, `options.json`, `log()`, `logJson()` and
the `client.query(...)` call to ensure strictly JSON output in that case.
Comment on lines
+55
to
+75
| const handleCleanup = async () => { | ||
| rl.close() | ||
| await cleanup() | ||
| } | ||
|
|
||
| process.on('SIGINT', () => { | ||
| if (buffer) { | ||
| // Cancel current multi-line input | ||
| buffer = '' | ||
| process.stdout.write('\n') | ||
| rl.setPrompt('netlifydb=> ') | ||
| rl.prompt() | ||
| } else { | ||
| log('') | ||
| void handleCleanup() | ||
| } | ||
| }) | ||
|
|
||
| rl.on('close', () => { | ||
| void handleCleanup() | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the re-entrant cleanup path in connect.ts and non-idempotent cleanup in db-connection.ts.
rg -n -C2 "const handleCleanup|rl\\.on\\('close'|rl\\.once\\('close'|process\\.on\\('SIGINT'|await cleanup\\(\\)|rl\\.close\\(\\)" src/commands/database/connect.ts
rg -n -C2 "cleanup:\\s*async|client\\.end\\(\\)|netlifyDev\\.stop\\(\\)" src/commands/database/db-connection.tsRepository: netlify/cli
Length of output: 1106
🏁 Script executed:
# Check the netlifyDev.stop() implementation to assess idempotency
rg -n "stop\s*\(" src/commands/dev/index.ts | head -20
rg -n "netlifyDev\s*=" src/commands/database/db-connection.ts -A 2
fd -t f -name "*.ts" -path "*/dev*" | xargs rg -l "export.*stop" | head -5Repository: netlify/cli
Length of output: 430
🏁 Script executed:
# Check if cleanup() implementations have guards or are called multiple times elsewhere
rg -n "cleanup\(\)" src/commands/database/connect.ts
rg -n "\.end\(\)" src/commands/database/db-connection.ts -B 2 -A 1Repository: netlify/cli
Length of output: 359
🏁 Script executed:
# Find the NetlifyDev class definition
rg -n "class NetlifyDev|export.*NetlifyDev" src/ --type tsRepository: netlify/cli
Length of output: 142
🏁 Script executed:
# Find the stop method in NetlifyDev
fd -type f -name "*.ts" | xargs rg -l "NetlifyDev" | head -5Repository: netlify/cli
Length of output: 350
🏁 Script executed:
# Check if netlifyDev.stop() has any idempotency checks or state guards
rg -n "stop\s*\(.*\)\s*{" src/ --type ts -A 5 | grep -E "stop|state|running" | head -20Repository: netlify/cli
Length of output: 152
🏁 Script executed:
# Search for the stop method implementation
rg -n "stop\s*\(" src/commands/dev/ -A 5 | head -40Repository: netlify/cli
Length of output: 410
🏁 Script executed:
# Check the NetlifyDev class and its stop method
cat src/commands/dev/programmatic-netlify-dev.ts | head -100Repository: netlify/cli
Length of output: 1973
🏁 Script executed:
# Look for any state or guard checks in the dev command
rg -n "running|stopped|isRunning" src/commands/dev/ | head -20Repository: netlify/cli
Length of output: 37
🏁 Script executed:
# Check how handleCleanup is actually called and if there's error handling
rg -n "handleCleanup" src/commands/database/connect.ts -B 2 -A 2Repository: netlify/cli
Length of output: 474
🏁 Script executed:
# Verify Node.js readline behavior: check if there are any existing guards or patterns
rg -n "rl\.on|rl\.once" src/commands/database/connect.tsRepository: netlify/cli
Length of output: 121
🏁 Script executed:
# Check if there are any try-catch blocks around cleanup or stop calls
rg -n "try|catch|error" src/commands/database/db-connection.ts -A 1 -B 1 | head -30Repository: netlify/cli
Length of output: 37
🏁 Script executed:
# Check if there's any error handling when cleanup is called or if it validates state
rg -n "void handleCleanup" src/commands/database/connect.ts -B 3 -A 3Repository: netlify/cli
Length of output: 266
🏁 Script executed:
# Look at the full context of line 55-86 to understand the complete flow
sed -n '50,90p' src/commands/database/connect.tsRepository: netlify/cli
Length of output: 994
🏁 Script executed:
# Check the actual implementation of cleanup in both cases - how is it destructured/used?
sed -n '30,45p' src/commands/database/db-connection.ts
sed -n '65,75p' src/commands/database/db-connection.tsRepository: netlify/cli
Length of output: 647
Ensure cleanup runs exactly once; current flow executes it twice.
The handleCleanup() function at line 55-58 calls rl.close(), which emits the 'close' event and triggers the handler at line 73-75—causing handleCleanup() to execute again. Both calls use void, so they run concurrently. While PostgreSQL's client.end() is idempotent, the netlifyDev.stop() call in line 70-73 can fail or cause side effects when executed twice.
Proposed fix
- const handleCleanup = async () => {
- rl.close()
- await cleanup()
- }
+ let cleanupPromise: Promise<void> | undefined
+ const handleCleanup = () => {
+ cleanupPromise ??= (async () => {
+ process.off('SIGINT', onSigint)
+ rl.close()
+ await cleanup()
+ })()
+ return cleanupPromise
+ }
- process.on('SIGINT', () => {
+ const onSigint = () => {
if (buffer) {
// Cancel current multi-line input
buffer = ''
process.stdout.write('\n')
rl.setPrompt('netlifydb=> ')
rl.prompt()
} else {
log('')
void handleCleanup()
}
- })
+ }
+ process.on('SIGINT', onSigint)
- rl.on('close', () => {
+ rl.once('close', () => {
void handleCleanup()
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleCleanup = async () => { | |
| rl.close() | |
| await cleanup() | |
| } | |
| process.on('SIGINT', () => { | |
| if (buffer) { | |
| // Cancel current multi-line input | |
| buffer = '' | |
| process.stdout.write('\n') | |
| rl.setPrompt('netlifydb=> ') | |
| rl.prompt() | |
| } else { | |
| log('') | |
| void handleCleanup() | |
| } | |
| }) | |
| rl.on('close', () => { | |
| void handleCleanup() | |
| }) | |
| let cleanupPromise: Promise<void> | undefined | |
| const handleCleanup = () => { | |
| cleanupPromise ??= (async () => { | |
| process.off('SIGINT', onSigint) | |
| rl.close() | |
| await cleanup() | |
| })() | |
| return cleanupPromise | |
| } | |
| const onSigint = () => { | |
| if (buffer) { | |
| // Cancel current multi-line input | |
| buffer = '' | |
| process.stdout.write('\n') | |
| rl.setPrompt('netlifydb=> ') | |
| rl.prompt() | |
| } else { | |
| log('') | |
| void handleCleanup() | |
| } | |
| } | |
| process.on('SIGINT', onSigint) | |
| rl.once('close', () => { | |
| void handleCleanup() | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/database/connect.ts` around lines 55 - 75, The cleanup flow runs
twice because handleCleanup() calls rl.close(), which emits 'close' and triggers
the rl.on('close') handler to call handleCleanup() again; fix by making cleanup
idempotent or by preventing the duplicate invocation: add a module-level boolean
flag (e.g., cleanedUp) that handleCleanup(), the SIGINT handler, and the
rl.on('close') handler check and set so cleanup logic (calling cleanup(),
netlifyDev.stop(), client.end(), etc.) executes only once, and remove the call
to rl.close() from handleCleanup() or call rl.close() only when cleanedUp is
false; reference functions/variables: handleCleanup, rl.close,
process.on('SIGINT'), rl.on('close'), cleanup, netlifyDev.stop.
paulo
approved these changes
Apr 6, 2026
eduardoboucas
pushed a commit
that referenced
this pull request
Apr 6, 2026
🤖 I have created a release *beep* *boop* --- ## [24.10.0](v24.9.0...v24.10.0) (2026-04-06) ### Features * add `db connect` command ([#8134](#8134)) ([6742b4d](6742b4d)) ### Bug Fixes * **deps:** bump picomatch in /site ([#8100](#8100)) ([ee9b89b](ee9b89b)) * **deps:** bump smol-toml from 1.6.0 to 1.6.1 in /site ([#8098](#8098)) ([42bac16](42bac16)) --- 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes https://linear.app/netlify/issue/RUN-2706/cli-connect-command.