fix(api): remove unused CLI dependencies #1076
Conversation
WalkthroughThe changes remove the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI (cli.ts)
participant NestAppContext
participant LoggerService
participant MigrationCommand
User->>CLI (cli.ts): Run CLI with command and args
CLI (cli.ts)->>NestAppContext: Create context (logging disabled)
CLI (cli.ts)->>LoggerService: Resolve LoggerService
CLI (cli.ts)->>CLI (cli.ts): Check allowed commands
alt Command is 'migration'
CLI (cli.ts)->>MigrationCommand: Resolve and run(args)
else Invalid command
CLI (cli.ts)->>LoggerService: Log error
CLI (cli.ts)->>CLI (cli.ts): Exit(1)
end
CLI (cli.ts)->>NestAppContext: Close context
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (1)
api/src/cli.ts (1)
20-20: Consider a more extensible command architecture for future growth.The current hard-coded approach works for the single migration command but may need refactoring as more commands are added. Consider implementing a command registry pattern for better maintainability.
For future enhancements, consider:
- A command registry with auto-discovery
- Help/usage generation
- Command-specific argument parsing
- Modular command structure
This can be addressed in future iterations as the CLI grows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
api/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
api/package.json(0 hunks)api/src/cli.ts(1 hunks)api/src/migration/migration.command.ts(1 hunks)
💤 Files with no reviewable changes (1)
- api/package.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
api/src/cli.ts (1)
api/src/migration/migration.command.ts (1)
MigrationCommand(16-66)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: API-Tests
- GitHub Check: Frontend Tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
api/src/migration/migration.command.ts (1)
16-16:✅ Verification successful
Ensure MigrationCommand is registered as a provider.
Since this class is no longer using nest-commander's automatic registration, verify that
MigrationCommandis properly registered as a provider in the relevant module to enableappContext.get(MigrationCommand)incli.ts.Run the following script to verify the provider registration:
🏁 Script executed:
#!/bin/bash # Description: Check if MigrationCommand is registered as a provider in any module # Search for MigrationCommand in provider arrays rg -A 10 -B 5 "providers.*\[" | rg -A 15 -B 5 "MigrationCommand"Length of output: 1250
MigrationCommand registration verified
MigrationCommand is already registered in
api/src/migration/migration.module.tsunder theprovidersarray alongsideMigrationService. No further changes are needed.api/src/cli.ts (2)
24-27: Good practice: Clean application context creation for CLI.The application context creation with disabled logging is appropriate for CLI usage, and the manual logger resolution ensures proper logging functionality.
36-36: Proper cleanup: Application context is correctly closed.Good practice to ensure the application context is properly closed after command execution to prevent resource leaks.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
api/src/cli.ts (1)
34-39: Improve error type safety in catch block.The error handling is present (addressing the previous review concern), but could be more type-safe.
} catch (error) { - logger.error(`Migration command failed: ${error.message}`); + logger.error(`Migration command failed: ${error instanceof Error ? error.message : String(error)}`); process.exit(1); }This ensures proper error message extraction regardless of the error type.
🧹 Nitpick comments (1)
api/src/cli.ts (1)
20-20: Consider making ALLOWED_COMMANDS more maintainable.While the current approach works for a single command, consider making this more scalable for future command additions.
-const ALLOWED_COMMANDS = ['migration']; +const ALLOWED_COMMANDS = ['migration'] as const; +type AllowedCommand = typeof ALLOWED_COMMANDS[number];This provides better type safety and makes it clear this is an immutable list.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/src/cli.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
api/src/cli.ts (2)
api/src/migration/migration.command.ts (1)
MigrationCommand(16-66)api/src/logger/logger.service.ts (1)
error(32-34)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: API-Tests
- GitHub Check: Frontend Tests
🔇 Additional comments (3)
api/src/cli.ts (3)
9-9: LGTM: Clean import structure after removing nest-commander.The imports are well-organized and reflect the migration from nest-commander to manual NestJS dependency injection.
Also applies to: 17-18
23-26: Good implementation of manual command parsing and app context creation.The approach correctly extracts command and arguments from process.argv and creates the NestJS context with logging disabled for CLI usage.
41-41: LGTM: Proper resource cleanup.The application context is properly closed, ensuring clean resource management.
Motivation
The main motivation of this PR is to remove a third party nest-commander package used from the CLI.
Fixes #1069
Checklist:
Summary by CodeRabbit