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:
📝 WalkthroughWalkthroughAdds a new nested CLI command group Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
📝 Coding Plan for PR comments
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 |
📊 Benchmark resultsComparing with 08aff83
|
paulo
force-pushed
the
pa/migration-new-command
branch
from
05aee9a to
7adc066
Compare
paulo
force-pushed
the
pa/migration-new-command
branch
from
7adc066 to
182fe9e
Compare
![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
🤖 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/migration-new.ts`:
- Around line 149-151: The code currently allows descriptions that slugify to an
empty string (e.g., "!!!" or "___"), producing invalid folder names like
`${prefix}_`; after calling generateSlug(description) in the migration-new flow,
validate the returned slug and if it's empty (or only whitespace) reject the
input: surface a clear error message and abort (throw or process.exit with
non-zero) rather than proceeding to build folderName; update any user-facing
error text to mention the original description and suggest a valid
alphanumeric/hyphenated description. Ensure you update the logic around
generateSlug, generateNextPrefix, and folderName so the check runs immediately
after generateSlug(description) and before using prefix/creating the folder.
- Around line 60-65: The code that computes the next prefix (using existingNames
-> prefixes -> maxPrefix) currently uses /^(\d+)/ which also matches
timestamp-style folders; update the matching logic so when computing sequential
prefixes you only consider 4-digit sequential prefixes (e.g. use a regex like
/^(\d{4})/ to extract the four-digit prefix) so timestamp names (e.g.
20260312143000_add-users) are ignored, then continue to parseInt, compute
maxPrefix and padStart as before.
- Around line 154-155: The current code unconditionally creates the folder and
then calls writeFile(join(folderPath, 'migration.sql'), ...) which will silently
overwrite an existing migration; update migration-new.ts so you do not clobber
user edits: after mkdir(folderPath, { recursive: true }) check whether the
target file (join(folderPath, 'migration.sql')) already exists and, if it does,
abort with a clear error message; alternatively replace the writeFile call with
writeFile(..., { flag: 'wx' }) so the write fails if the file exists. Reference
the existing symbols mkdir, writeFile, folderPath and the 'migration.sql'
filename when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce3eb328-c6ba-44df-b7a7-8550b98629a2
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
src/commands/database/database.tssrc/commands/database/db-connection.tssrc/commands/database/migration-new.tstests/unit/commands/database/migration-new.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/commands/database/migration-new.test.ts (1)
82-98: Consider adding a test for mixed numbering patterns.The current tests cover pure sequential, pure timestamp, and unrecognized patterns, but not a mix of valid sequential and timestamp migrations in the same directory.
🧪 Suggested test case
test('returns undefined for mixed sequential and timestamp patterns', () => { expect(detectNumberingScheme(['0001_create-users', '20260312143000_add-posts'])).toBeUndefined() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/commands/database/migration-new.test.ts` around lines 82 - 98, Add a test to cover mixed numbering patterns so detectNumberingScheme returns undefined when both sequential and timestamp-style names are present; update tests/unit/commands/database/migration-new.test.ts by adding a new test case that calls detectNumberingScheme(['0001_create-users', '20260312143000_add-posts']) and expects undefined, ensuring the suite checks mixed-pattern detection in the detectNumberingScheme unit tests.src/commands/database/migration-new.ts (1)
118-147: Consider deduplicating the prompt configuration.The two
inquirer.promptcalls for scheme selection (lines 120-132 and 134-145) share nearly identical configuration, differing only by the presence ofdefault.♻️ Suggested refactor
if (!scheme) { - if (detectedScheme) { - const answers = await inquirer.prompt<{ scheme: NumberingScheme }>([ - { - type: 'list', - name: 'scheme', - message: 'Numbering scheme:', - choices: [ - { name: 'Sequential (0001, 0002, ...)', value: 'sequential' }, - { name: 'Timestamp (20260312143000)', value: 'timestamp' }, - ], - default: detectedScheme, - }, - ]) - scheme = answers.scheme - } else { - const answers = await inquirer.prompt<{ scheme: NumberingScheme }>([ - { - type: 'list', - name: 'scheme', - message: 'Numbering scheme:', - choices: [ - { name: 'Sequential (0001, 0002, ...)', value: 'sequential' }, - { name: 'Timestamp (20260312143000)', value: 'timestamp' }, - ], - }, - ]) - scheme = answers.scheme - } + const answers = await inquirer.prompt<{ scheme: NumberingScheme }>([ + { + type: 'list', + name: 'scheme', + message: 'Numbering scheme:', + choices: [ + { name: 'Sequential (0001, 0002, ...)', value: 'sequential' }, + { name: 'Timestamp (20260312143000)', value: 'timestamp' }, + ], + ...(detectedScheme && { default: detectedScheme }), + }, + ]) + scheme = answers.scheme }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/database/migration-new.ts` around lines 118 - 147, The two inquirer.prompt calls for selecting the numbering scheme in migration-new.ts are duplicated; consolidate them by building a single question object (or questions array) with the shared properties (type, name, message, choices) and only add a default property when detectedScheme is truthy, then call inquirer.prompt once to set scheme; reference the variables and symbols scheme, detectedScheme, NumberingScheme, and the inquirer.prompt invocation when making the change.
🤖 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/migration-new.ts`:
- Around line 118-147: The two inquirer.prompt calls for selecting the numbering
scheme in migration-new.ts are duplicated; consolidate them by building a single
question object (or questions array) with the shared properties (type, name,
message, choices) and only add a default property when detectedScheme is truthy,
then call inquirer.prompt once to set scheme; reference the variables and
symbols scheme, detectedScheme, NumberingScheme, and the inquirer.prompt
invocation when making the change.
In `@tests/unit/commands/database/migration-new.test.ts`:
- Around line 82-98: Add a test to cover mixed numbering patterns so
detectNumberingScheme returns undefined when both sequential and timestamp-style
names are present; update tests/unit/commands/database/migration-new.test.ts by
adding a new test case that calls detectNumberingScheme(['0001_create-users',
'20260312143000_add-posts']) and expects undefined, ensuring the suite checks
mixed-pattern detection in the detectNumberingScheme unit tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 79f44c13-c311-4c09-b40e-ff497c599911
📒 Files selected for processing (2)
src/commands/database/migration-new.tstests/unit/commands/database/migration-new.test.ts
| } | ||
|
|
||
| if (!scheme) { | ||
| if (detectedScheme) { |
There was a problem hiding this comment.
I don't think we need to repeat the whole inquirer.prompt call in the two branches if the whole thing that differs is the default property?
| const migrationFilePath = join(folderPath, 'migration.sql') | ||
|
|
||
| await mkdir(folderPath, { recursive: true }) | ||
| await writeFile(migrationFilePath, '-- Write your migration SQL here\n', { flag: 'wx' }) |
There was a problem hiding this comment.
Should we include a basic CREATE IF NOT EXISTS example in this comment, just to give a little bit more detail about what's expected here?
2 participants
-)