Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request adds a new database reset command to the CLI. Changes include bumping the Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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. 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: 1
🧹 Nitpick comments (1)
src/commands/database/database.ts (1)
101-104: Add an explicit confirmation path fordb reset.This subcommand drops the entire local schema immediately, but there’s no confirmation or
--forceescape hatch. Prompting in interactive shells and requiring--forcefor non-interactive runs would make accidental data loss much harder.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/database/database.ts` around lines 101 - 104, Add an explicit confirmation flow to the 'reset' subcommand: introduce a boolean option '--force' and, inside the .action handler for the reset command (the async (options: { json?: boolean }, command: BaseCommand) => { ... } block), if stdin is a TTY prompt the user to type an explicit confirmation (e.g., "type RESET to confirm") before proceeding; if stdin is not a TTY, require that options.force is true or abort with a clear message. Ensure the prompt branch prevents deletion on cancel and that the non-interactive branch only proceeds when --force is present, and reuse the existing command/console utilities for prompting and aborting so behavior integrates with the rest of the CLI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/commands/database/database.ts`:
- Around line 101-104: Add an explicit confirmation flow to the 'reset'
subcommand: introduce a boolean option '--force' and, inside the .action handler
for the reset command (the async (options: { json?: boolean }, command:
BaseCommand) => { ... } block), if stdin is a TTY prompt the user to type an
explicit confirmation (e.g., "type RESET to confirm") before proceeding; if
stdin is not a TTY, require that options.force is true or abort with a clear
message. Ensure the prompt branch prevents deletion on cancel and that the
non-interactive branch only proceeds when --force is present, and reuse the
existing command/console utilities for prompting and aborting so behavior
integrates with the rest of the CLI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6b3fd70-8750-495b-a021-91a8a7a6f335
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonsrc/commands/database/database.tssrc/commands/database/reset.tstests/unit/commands/database/reset.test.ts
| const netlifyDev = new NetlifyDev({ | ||
| projectRoot: buildDir, | ||
| aiGateway: { enabled: false }, | ||
| blobs: { enabled: false }, | ||
| edgeFunctions: { enabled: false }, | ||
| environmentVariables: { enabled: false }, | ||
| functions: { enabled: false }, | ||
| geolocation: { enabled: false }, | ||
| headers: { enabled: false }, | ||
| images: { enabled: false }, | ||
| redirects: { enabled: false }, | ||
| staticFiles: { enabled: false }, | ||
| serverAddress: null, | ||
| }) | ||
|
|
||
| try { | ||
| await netlifyDev.start() | ||
|
|
||
| const { db } = netlifyDev | ||
| if (!db) { | ||
| throw new Error('Local database failed to start. Set EXPERIMENTAL_NETLIFY_DB_ENABLED=1 to enable.') | ||
| } | ||
|
|
||
| await db.reset() |
There was a problem hiding this comment.
Block db reset while another netlify dev session is using the local DB.
The PR description already calls out that running reset against a separate PGlite process can leave the data directory corrupted when a dev server is active. This code path always spins up a new NetlifyDev instance and proceeds to db.reset() without checking for that state, so the known corruption case is still user-reachable. Please fail fast here until the command can coordinate with the running dev server.
There was a problem hiding this comment.
Ohh interesting, the bot detected it as well. I'll address this in a follow-up, because it also affects the migrations.
There was a problem hiding this comment.
@paulo — Sounds good, thanks for confirming! Since this affects both db reset and migrations, it might be worth tracking as a GitHub issue so it doesn't get lost. Would you like me to open one for it?
2 participants
Summary
This adds the reset command from the primitives repo. However, I've noted a couple of issues that I'll address in a follow-up: